summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLuca Milanesio <luca.milanesio@gmail.com>2022-10-14 21:12:48 +0000
committerGerrit Code Review <noreply-gerritcodereview@google.com>2022-10-14 21:12:48 +0000
commit436905c9a9a2895519bcb837a58fdb7750a676c0 (patch)
treee33f400357bdf5e66b34b43959240c0c89b57f48
parentcf1ce3f69602e7c2dacfe568c9194117721cddae (diff)
parentc6737c9d629bf5f660ae775091172a7b0a44262a (diff)
Merge changes from topic "import-changes" into stable-3.7
* changes: Resolve accounts by imported externalId Introduce imported external ids for accounts from other servers Allow the rendering of changes imported from other GerritServerIds
-rw-r--r--Documentation/config-gerrit.txt20
-rw-r--r--java/com/google/gerrit/entities/Account.java4
-rw-r--r--java/com/google/gerrit/server/account/externalids/ExternalId.java3
-rw-r--r--java/com/google/gerrit/server/account/externalids/ExternalIdCache.java2
-rw-r--r--java/com/google/gerrit/server/config/GerritImportedServerIds.java29
-rw-r--r--java/com/google/gerrit/server/config/GerritImportedServerIdsProvider.java37
-rw-r--r--java/com/google/gerrit/server/notedb/AbstractChangeNotes.java7
-rw-r--r--java/com/google/gerrit/server/notedb/ChangeNotes.java8
-rw-r--r--java/com/google/gerrit/server/notedb/ChangeNotesCache.java16
-rw-r--r--java/com/google/gerrit/server/notedb/ChangeNotesParser.java11
-rw-r--r--java/com/google/gerrit/server/notedb/NoteDbUtil.java43
-rw-r--r--java/com/google/gerrit/server/schema/SchemaModule.java9
-rw-r--r--java/com/google/gerrit/testing/InMemoryModule.java14
-rw-r--r--javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java157
-rw-r--r--javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java3
-rw-r--r--javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java11
-rw-r--r--javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java16
-rw-r--r--javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java114
18 files changed, 436 insertions, 68 deletions
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 234389e2e7..be6e140f33 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2543,6 +2543,26 @@ Rolling upgrade may or may not be possible depending on the changes introduced
by the target version of the upgrade. Refer to the release notes and check whether
the rolling upgrade is possible or not and the associated constraints.
+[[gerrit.importedServerId]]gerrit.importedServerId::
++
+ServerId of the repositories imported from other Gerrit servers. Changes coming
+associated with the imported serverIds are indexed and displayed in the UI.
++
+Specify multiple `gerrit.importedServerId` for allowing the import from multiple
+Gerrit servers with different serverIds.
++
+[NOTE]
+The account-ids referenced in the imported changes are used for looking up the
+associated account-id locally, using the `imported:` external-id.
+Example: the account-id 1000 from the imported server-id 59a4964e-6376-4ed9-beef
+will be looked up in the local accounts using the `imported:1000@59a4964e-6376-4ed9-beef`
+external-id.
++
+If this value is not set, all changes imported from other Gerrit servers will be
+ignored.
++
+By default empty.
+
[[gerrit.serverId]]gerrit.serverId::
+
Used by NoteDb to, amongst other things, identify author identities from
diff --git a/java/com/google/gerrit/entities/Account.java b/java/com/google/gerrit/entities/Account.java
index 303e79fa96..85dbdeb2b4 100644
--- a/java/com/google/gerrit/entities/Account.java
+++ b/java/com/google/gerrit/entities/Account.java
@@ -46,6 +46,10 @@ import java.util.Optional;
*/
@AutoValue
public abstract class Account {
+
+ /** Placeholder for indicating an account-id that does not correspond to any local account */
+ public static final Id UNKNOWN_ACCOUNT_ID = id(0);
+
public static Id id(int id) {
return new AutoValue_Account_Id(id);
}
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalId.java b/java/com/google/gerrit/server/account/externalids/ExternalId.java
index 0a51171a3a..161619853c 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalId.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalId.java
@@ -135,6 +135,9 @@ public abstract class ExternalId implements Serializable {
/** Scheme used for GPG public keys. */
public static final String SCHEME_GPGKEY = "gpgkey";
+ /** Scheme for imported accounts from other servers with different GerritServerId */
+ public static final String SCHEME_IMPORTED = "imported";
+
/** Scheme for external auth used during authentication, e.g. OAuth Token */
public static final String SCHEME_EXTERNAL = "external";
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
index 9f766d0922..fe8feac867 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
@@ -30,7 +30,7 @@ import org.eclipse.jgit.lib.ObjectId;
*
* <p>All returned collections are unmodifiable.
*/
-interface ExternalIdCache {
+public interface ExternalIdCache {
Optional<ExternalId> byKey(ExternalId.Key key) throws IOException;
ImmutableSet<ExternalId> byAccount(Account.Id accountId) throws IOException;
diff --git a/java/com/google/gerrit/server/config/GerritImportedServerIds.java b/java/com/google/gerrit/server/config/GerritImportedServerIds.java
new file mode 100644
index 0000000000..c47d3be7bb
--- /dev/null
+++ b/java/com/google/gerrit/server/config/GerritImportedServerIds.java
@@ -0,0 +1,29 @@
+// Copyright (C) 2022 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.config;
+
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import com.google.inject.BindingAnnotation;
+import java.lang.annotation.Retention;
+
+/**
+ * List of ServerIds of the Gerrit data imported from other servers.
+ *
+ * <p>This values correspond to the {@code GerritServerId} of other servers.
+ */
+@Retention(RUNTIME)
+@BindingAnnotation
+public @interface GerritImportedServerIds {}
diff --git a/java/com/google/gerrit/server/config/GerritImportedServerIdsProvider.java b/java/com/google/gerrit/server/config/GerritImportedServerIdsProvider.java
new file mode 100644
index 0000000000..f3f76456ce
--- /dev/null
+++ b/java/com/google/gerrit/server/config/GerritImportedServerIdsProvider.java
@@ -0,0 +1,37 @@
+// Copyright (C) 2022 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.config;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import org.eclipse.jgit.lib.Config;
+
+public class GerritImportedServerIdsProvider implements Provider<ImmutableSet<String>> {
+ public static final String SECTION = "gerrit";
+ public static final String KEY = "importedServerId";
+
+ private final ImmutableSet<String> importedIds;
+
+ @Inject
+ public GerritImportedServerIdsProvider(@GerritServerConfig Config cfg) {
+ importedIds = ImmutableSet.copyOf(cfg.getStringList(SECTION, null, KEY));
+ }
+
+ @Override
+ public ImmutableSet<String> get() {
+ return importedIds;
+ }
+}
diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index 158972fa0c..3757244f8f 100644
--- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Change;
@@ -24,6 +25,7 @@ import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.GerritImportedServerIds;
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
@@ -51,6 +53,7 @@ public abstract class AbstractChangeNotes<T> {
public final AllUsersName allUsers;
public final NoteDbMetrics metrics;
public final String serverId;
+ public final ImmutableSet<String> importedServerIds;
// Providers required to avoid dependency cycles.
@@ -64,7 +67,8 @@ public abstract class AbstractChangeNotes<T> {
ChangeNoteJson changeNoteJson,
NoteDbMetrics metrics,
Provider<ChangeNotesCache> cache,
- @GerritServerId String serverId) {
+ @GerritServerId String serverId,
+ @GerritImportedServerIds ImmutableSet<String> importedServerIds) {
this.failOnLoadForTest = new AtomicBoolean();
this.repoManager = repoManager;
this.allUsers = allUsers;
@@ -72,6 +76,7 @@ public abstract class AbstractChangeNotes<T> {
this.metrics = metrics;
this.cache = cache;
this.serverId = serverId;
+ this.importedServerIds = importedServerIds;
}
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 26d593332b..31934d53e6 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -406,6 +406,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return state.metaId();
}
+ public String getServerId() {
+ return state.serverId();
+ }
+
public ImmutableSortedMap<PatchSet.Id, PatchSet> getPatchSets() {
if (patchSets == null) {
ImmutableSortedMap.Builder<PatchSet.Id, PatchSet> b =
@@ -655,7 +659,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
* be to bump the cache version, but that would invalidate all persistent cache entries, what we
* rather try to avoid.
*/
- if (!Strings.isNullOrEmpty(stateServerId) && !args.serverId.equals(stateServerId)) {
+ if (!Strings.isNullOrEmpty(stateServerId)
+ && !args.serverId.equals(stateServerId)
+ && !args.importedServerIds.contains(stateServerId)) {
throw new InvalidServerIdException(args.serverId, stateServerId);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index 19044e693f..0f2c8778e1 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -26,6 +26,7 @@ import com.google.gerrit.entities.RefNames;
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.cache.proto.Cache.ChangeNotesKeyProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
@@ -366,7 +367,13 @@ public class ChangeNotesCache {
"Load change notes for change %s of project %s", key.changeId(), key.project());
ChangeNotesParser parser =
new ChangeNotesParser(
- key.changeId(), key.id(), walkSupplier.get(), args.changeNoteJson, args.metrics);
+ key.changeId(),
+ key.id(),
+ walkSupplier.get(),
+ args.changeNoteJson,
+ args.metrics,
+ 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.
@@ -377,11 +384,16 @@ public class ChangeNotesCache {
private final Cache<Key, ChangeNotesState> cache;
private final Args args;
+ private final ExternalIdCache externalIdCache;
@Inject
- ChangeNotesCache(@Named(CACHE_NAME) Cache<Key, ChangeNotesState> cache, Args args) {
+ ChangeNotesCache(
+ @Named(CACHE_NAME) Cache<Key, ChangeNotesState> cache,
+ Args args,
+ ExternalIdCache externalIdCache) {
this.cache = cache;
this.args = args;
+ this.externalIdCache = externalIdCache;
}
Value get(
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 6e5e19c022..f2a659dc5f 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -79,6 +79,7 @@ import com.google.gerrit.server.AssigneeStatusUpdate;
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.ChangeNoteUtil.ParsedPatchSetApproval;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.util.LabelVote;
@@ -199,18 +200,24 @@ class ChangeNotesParser {
// the latest record unsets the field).
private Optional<PatchSet.Id> cherryPickOf;
private Instant mergedOn;
+ private final ExternalIdCache externalIdCache;
+ private final String gerritServerId;
ChangeNotesParser(
Change.Id changeId,
ObjectId tip,
ChangeNotesRevWalk walk,
ChangeNoteJson changeNoteJson,
- NoteDbMetrics metrics) {
+ NoteDbMetrics metrics,
+ String gerritServerId,
+ ExternalIdCache externalIdCache) {
this.id = changeId;
this.tip = tip;
this.walk = walk;
this.changeNoteJson = changeNoteJson;
this.metrics = metrics;
+ this.externalIdCache = externalIdCache;
+ this.gerritServerId = gerritServerId;
approvals = new LinkedHashMap<>();
bufferedApprovals = new ArrayList<>();
reviewers = HashBasedTable.create();
@@ -1406,7 +1413,7 @@ class ChangeNotesParser {
}
private Account.Id parseIdent(PersonIdent ident) throws ConfigInvalidException {
- return NoteDbUtil.parseIdent(ident)
+ return NoteDbUtil.parseIdent(ident, gerritServerId, externalIdCache)
.orElseThrow(
() -> parseException("cannot retrieve account id: %s", ident.getEmailAddress()));
}
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUtil.java b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
index 396e29b5ce..64bf430b05 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUtil.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
@@ -20,8 +20,12 @@ import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
+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;
@@ -48,6 +52,45 @@ public class NoteDbUtil {
return Optional.empty();
}
+ /**
+ * 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
+ * @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();
+ int at = email.indexOf('@');
+ if (at >= 0) {
+ Integer id = Ints.tryParse(email.substring(0, at));
+ String accountServerId = email.substring(at + 1);
+ if (id != null) {
+ if (accountServerId.equals(serverId)) {
+ return Optional.of(Account.id(id));
+ }
+
+ ExternalId.Key extIdKey = ExternalId.Key.create(ExternalId.SCHEME_IMPORTED, email, false);
+ try {
+ return externalIdCache
+ .byKey(extIdKey)
+ .map(ExternalId::accountId)
+ .or(() -> Optional.of(Account.UNKNOWN_ACCOUNT_ID));
+ } catch (IOException e) {
+ throw new ConfigInvalidException("Unable to lookup external id from cache", e);
+ }
+ }
+ }
+ return Optional.empty();
+ }
+
public static String extractHostPartFromPersonIdent(PersonIdent ident) {
String email = ident.getEmailAddress();
int at = email.indexOf('@');
diff --git a/java/com/google/gerrit/server/schema/SchemaModule.java b/java/com/google/gerrit/server/schema/SchemaModule.java
index ff2073dddc..e0e64a31e8 100644
--- a/java/com/google/gerrit/server/schema/SchemaModule.java
+++ b/java/com/google/gerrit/server/schema/SchemaModule.java
@@ -16,6 +16,7 @@ package com.google.gerrit.server.schema;
import static com.google.inject.Scopes.SINGLETON;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.GerritPersonIdentProvider;
@@ -25,9 +26,12 @@ import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
+import com.google.gerrit.server.config.GerritImportedServerIds;
+import com.google.gerrit.server.config.GerritImportedServerIdsProvider;
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.config.GerritServerIdProvider;
import com.google.gerrit.server.index.group.GroupIndexCollection;
+import com.google.inject.TypeLiteral;
import org.eclipse.jgit.lib.PersonIdent;
/** Bindings for low-level Gerrit schema data. */
@@ -51,6 +55,11 @@ public class SchemaModule extends FactoryModule {
.toProvider(GerritServerIdProvider.class)
.in(SINGLETON);
+ bind(new TypeLiteral<ImmutableSet<String>>() {})
+ .annotatedWith(GerritImportedServerIds.class)
+ .toProvider(GerritImportedServerIdsProvider.class)
+ .in(SINGLETON);
+
// It feels wrong to have this binding in a seemingly unrelated module, but it's a dependency of
// SchemaCreatorImpl, so it's needed.
// TODO(dborowitz): Is there any way to untangle this?
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index b00cadb7a7..b1ab6b1b9a 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -19,6 +19,7 @@ import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorS
import static com.google.inject.Scopes.SINGLETON;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperationsImpl;
@@ -61,6 +62,8 @@ import com.google.gerrit.server.config.DefaultUrlFormatter.DefaultUrlFormatterMo
import com.google.gerrit.server.config.FileBasedAllProjectsConfigProvider;
import com.google.gerrit.server.config.FileBasedGlobalPluginConfigProvider;
import com.google.gerrit.server.config.GerritGlobalModule;
+import com.google.gerrit.server.config.GerritImportedServerIds;
+import com.google.gerrit.server.config.GerritImportedServerIdsProvider;
import com.google.gerrit.server.config.GerritInstanceIdModule;
import com.google.gerrit.server.config.GerritInstanceNameModule;
import com.google.gerrit.server.config.GerritOptions;
@@ -311,6 +314,17 @@ public class InMemoryModule extends FactoryModule {
@Provides
@Singleton
+ @GerritImportedServerIds
+ public ImmutableSet<String> createImportedServerIds() {
+ ImmutableSet<String> serverIds =
+ ImmutableSet.copyOf(
+ cfg.getStringList(
+ GerritServerIdProvider.SECTION, null, GerritImportedServerIdsProvider.KEY));
+ return serverIds;
+ }
+
+ @Provides
+ @Singleton
@SendEmailExecutor
public ExecutorService createSendEmailExecutor() {
return newDirectExecutorService();
diff --git a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index d7c779e1f4..1b286d18eb 100644
--- a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -18,6 +18,7 @@ import static com.google.inject.Scopes.SINGLETON;
import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Comment;
@@ -39,6 +40,8 @@ import com.google.gerrit.server.account.FakeRealm;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.account.externalids.DisabledExternalIdCache;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.approval.PatchSetApprovalUuidGenerator;
import com.google.gerrit.server.approval.testing.TestPatchSetApprovalUuidGenerator;
import com.google.gerrit.server.config.AllUsersName;
@@ -48,11 +51,13 @@ import com.google.gerrit.server.config.AnonymousCowardNameProvider;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.DefaultUrlFormatter.DefaultUrlFormatterModule;
import com.google.gerrit.server.config.EnablePeerIPInReflogRecord;
+import com.google.gerrit.server.config.GerritImportedServerIds;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitModule;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.RepositoryCaseMismatchException;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.NullProjectCache;
import com.google.gerrit.server.project.ProjectCache;
@@ -67,9 +72,12 @@ import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
+import com.google.inject.Module;
+import com.google.inject.TypeLiteral;
import java.time.Instant;
import java.time.ZoneId;
import java.util.concurrent.ExecutorService;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
@@ -84,10 +92,13 @@ import org.junit.runner.RunWith;
@Ignore
@RunWith(ConfigSuite.class)
public abstract class AbstractChangeNotesTest {
+ protected static final String LOCAL_SERVER_ID = "gerrit";
+
private static final ZoneId ZONE_ID = ZoneId.of("America/Los_Angeles");
@ConfigSuite.Parameter public Config testConfig;
+ protected Account.Id changeOwnerId;
protected Account.Id otherUserId;
protected FakeAccountCache accountCache;
protected IdentifiedUser changeOwner;
@@ -111,11 +122,21 @@ public abstract class AbstractChangeNotesTest {
@Inject @GerritServerId protected String serverId;
+ @Inject protected ExternalIdCache externalIdCache;
+
protected Injector injector;
private String systemTimeZone;
@Before
public void setUpTestEnvironment() throws Exception {
+ setupTestPrerequisites();
+
+ injector = createTestInjector(LOCAL_SERVER_ID);
+ createAllUsers(injector);
+ injector.injectMembers(this);
+ }
+
+ protected void setupTestPrerequisites() throws Exception {
setTimeForTesting();
serverIdent = new PersonIdent("Gerrit Server", "noreply@gerrit.com", TimeUtil.now(), ZONE_ID);
@@ -134,58 +155,75 @@ public abstract class AbstractChangeNotesTest {
ou.setPreferredEmail("other@account.com");
accountCache.put(ou.build());
assertableFanOutExecutor = new AssertableExecutorService();
+ changeOwnerId = co.id();
+ otherUserId = ou.id();
+ internalUser = new InternalUser();
+ }
- injector =
- Guice.createInjector(
- new FactoryModule() {
- @Override
- public void configure() {
- install(new GitModule());
-
- install(new DefaultUrlFormatterModule());
- install(NoteDbModule.forTest());
- bind(AllUsersName.class).toProvider(AllUsersNameProvider.class);
- bind(String.class).annotatedWith(GerritServerId.class).toInstance("gerrit");
- bind(GitRepositoryManager.class).toInstance(repoManager);
- bind(ProjectCache.class).to(NullProjectCache.class);
- bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(testConfig);
- bind(String.class)
- .annotatedWith(AnonymousCowardName.class)
- .toProvider(AnonymousCowardNameProvider.class);
- bind(String.class)
- .annotatedWith(CanonicalWebUrl.class)
- .toInstance("http://localhost:8080/");
- bind(Boolean.class)
- .annotatedWith(EnablePeerIPInReflogRecord.class)
- .toInstance(Boolean.FALSE);
- bind(Realm.class).to(FakeRealm.class);
- bind(GroupBackend.class).to(SystemGroupBackend.class).in(SINGLETON);
- bind(AccountCache.class).toInstance(accountCache);
- bind(PersonIdent.class)
- .annotatedWith(GerritPersonIdent.class)
- .toInstance(serverIdent);
- bind(GitReferenceUpdated.class).toInstance(GitReferenceUpdated.DISABLED);
- bind(MetricMaker.class).to(DisabledMetricMaker.class);
- bind(ExecutorService.class)
- .annotatedWith(FanOutExecutor.class)
- .toInstance(assertableFanOutExecutor);
- bind(ServiceUserClassifier.class).to(ServiceUserClassifier.NoOp.class);
- bind(InternalChangeQuery.class)
- .toProvider(
- () -> {
- throw new UnsupportedOperationException();
- });
- bind(PatchSetApprovalUuidGenerator.class)
- .to(TestPatchSetApprovalUuidGenerator.class);
- }
- });
+ protected Injector createTestInjector(String serverId, String... importedServerIds)
+ throws Exception {
+ return createTestInjector(DisabledExternalIdCache.module(), serverId, importedServerIds);
+ }
- injector.injectMembers(this);
- repoManager.createRepository(allUsers);
- changeOwner = userFactory.create(co.id());
- otherUser = userFactory.create(ou.id());
- otherUserId = otherUser.getAccountId();
- internalUser = new InternalUser();
+ protected Injector createTestInjector(
+ Module extraGuiceModule, String serverId, String... importedServerIds) throws Exception {
+
+ return Guice.createInjector(
+ new FactoryModule() {
+ @Override
+ public void configure() {
+ install(extraGuiceModule);
+ install(new GitModule());
+
+ install(new DefaultUrlFormatterModule());
+ install(NoteDbModule.forTest());
+ bind(AllUsersName.class).toProvider(AllUsersNameProvider.class);
+ bind(String.class).annotatedWith(GerritServerId.class).toInstance(serverId);
+ bind(new TypeLiteral<ImmutableSet<String>>() {})
+ .annotatedWith(GerritImportedServerIds.class)
+ .toInstance(new ImmutableSet.Builder<String>().add(importedServerIds).build());
+ bind(GitRepositoryManager.class).toInstance(repoManager);
+ bind(ProjectCache.class).to(NullProjectCache.class);
+ bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(testConfig);
+ bind(String.class)
+ .annotatedWith(AnonymousCowardName.class)
+ .toProvider(AnonymousCowardNameProvider.class);
+ bind(String.class)
+ .annotatedWith(CanonicalWebUrl.class)
+ .toInstance("http://localhost:8080/");
+ bind(Boolean.class)
+ .annotatedWith(EnablePeerIPInReflogRecord.class)
+ .toInstance(Boolean.FALSE);
+ bind(Realm.class).to(FakeRealm.class);
+ bind(GroupBackend.class).to(SystemGroupBackend.class).in(SINGLETON);
+ bind(AccountCache.class).toInstance(accountCache);
+ bind(PersonIdent.class).annotatedWith(GerritPersonIdent.class).toInstance(serverIdent);
+ bind(GitReferenceUpdated.class).toInstance(GitReferenceUpdated.DISABLED);
+ bind(MetricMaker.class).to(DisabledMetricMaker.class);
+ bind(ExecutorService.class)
+ .annotatedWith(FanOutExecutor.class)
+ .toInstance(assertableFanOutExecutor);
+ bind(ServiceUserClassifier.class).to(ServiceUserClassifier.NoOp.class);
+ bind(InternalChangeQuery.class)
+ .toProvider(
+ () -> {
+ throw new UnsupportedOperationException();
+ });
+ bind(PatchSetApprovalUuidGenerator.class).to(TestPatchSetApprovalUuidGenerator.class);
+ }
+ });
+ }
+
+ protected void createAllUsers(Injector injector)
+ throws RepositoryCaseMismatchException, RepositoryNotFoundException {
+ AllUsersName allUsersName = injector.getInstance(AllUsersName.class);
+
+ repoManager.createRepository(allUsersName);
+
+ IdentifiedUser.GenericFactory identifiedUserFactory =
+ injector.getInstance(IdentifiedUser.GenericFactory.class);
+ changeOwner = identifiedUserFactory.create(changeOwnerId);
+ otherUser = identifiedUserFactory.create(otherUserId);
}
private void setTimeForTesting() {
@@ -200,8 +238,12 @@ public abstract class AbstractChangeNotesTest {
}
protected Change newChange(boolean workInProgress) throws Exception {
+ return newChange(injector, workInProgress);
+ }
+
+ protected Change newChange(Injector injector, boolean workInProgress) throws Exception {
Change c = TestChanges.newChange(project, changeOwner.getAccountId());
- ChangeUpdate u = newUpdateForNewChange(c, changeOwner);
+ ChangeUpdate u = newUpdate(injector, c, changeOwner, false);
u.setChangeId(c.getKey().get());
u.setBranch(c.getDest().branch());
u.setWorkInProgress(workInProgress);
@@ -218,15 +260,20 @@ public abstract class AbstractChangeNotesTest {
}
protected ChangeUpdate newUpdateForNewChange(Change c, CurrentUser user) throws Exception {
- return newUpdate(c, user, false);
+ return newUpdate(injector, c, user, false);
}
protected ChangeUpdate newUpdate(Change c, CurrentUser user) throws Exception {
- return newUpdate(c, user, true);
+ return newUpdate(injector, c, user, true);
}
protected ChangeUpdate newUpdate(Change c, CurrentUser user, boolean shouldExist)
throws Exception {
+ return newUpdate(injector, c, user, shouldExist);
+ }
+
+ protected ChangeUpdate newUpdate(
+ Injector injector, Change c, CurrentUser user, boolean shouldExist) throws Exception {
ChangeUpdate update = TestChanges.newUpdate(injector, c, user, shouldExist);
update.setPatchSetId(c.currentPatchSetId());
update.setAllowWriteToNewRef(true);
@@ -237,6 +284,10 @@ public abstract class AbstractChangeNotesTest {
return new ChangeNotes(args, c, true, null).load();
}
+ protected ChangeNotes newNotes(AbstractChangeNotes.Args cArgs, Change c) {
+ return new ChangeNotes(cArgs, c, true, null).load();
+ }
+
protected static SubmitRecord submitRecord(
String status, String errorMessage, SubmitRecord.Label... labels) {
SubmitRecord rec = new SubmitRecord();
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
index 666b8fcf4f..9445f4a858 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
@@ -98,7 +98,8 @@ public class ChangeNotesCommitTest extends AbstractChangeNotesTest {
private ChangeNotesParser newParser(ObjectId tip) throws Exception {
walk.reset();
ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class);
- return new ChangeNotesParser(newChange().getId(), tip, walk, changeNoteJson, args.metrics);
+ return new ChangeNotesParser(
+ newChange().getId(), tip, walk, changeNoteJson, args.metrics, 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 276f0931f2..4543b50c47 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
@@ -201,11 +201,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-set: 1\n"
- + "Copied-Label: Label1=+1 Account <1@gerrit>,Other Account <2@Gerrit>\n"
+ + "Copied-Label: Label1=+1 Account <1@gerrit>,Other Account <2@gerrit>\n"
+ "Copied-Label: Label2=+1 Account <1@gerrit>\n"
- + "Copied-Label: Label3=+1 Account <1@gerrit>,Other Account <2@Gerrit> :\"tag\"\n"
- + "Copied-Label: Label4=+1 Account <1@Gerrit> :\"tag with characters %^#@^( *::!\"\n"
- + "Copied-Label: -Label1 Account <1@gerrit>,Other Account <2@Gerrit>\\n"
+ + "Copied-Label: Label3=+1 Account <1@gerrit>,Other Account <2@gerrit> :\"tag\"\n"
+ + "Copied-Label: Label4=+1 Account <1@gerrit> :\"tag with characters %^#@^( *::!\"\n"
+ + "Copied-Label: -Label1 Account <1@gerrit>,Other Account <2@gerrit>\\n"
+ "Copied-Label: -Label1 Account <1@gerrit>\n"
+ "Subject: This is a test change\n");
@@ -782,6 +782,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
private ChangeNotesParser newParser(ObjectId tip) throws Exception {
walk.reset();
ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class);
- return new ChangeNotesParser(newChange().getId(), tip, walk, changeNoteJson, args.metrics);
+ return new ChangeNotesParser(
+ newChange().getId(), tip, walk, changeNoteJson, args.metrics, serverId, externalIdCache);
}
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 8def66004b..4edfa8b4e3 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -2084,7 +2084,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
ChangeNotesParser notesWithComments =
new ChangeNotesParser(
- c.getId(), commitWithComments.copy(), rw, changeNoteJson, args.metrics);
+ c.getId(),
+ commitWithComments.copy(),
+ rw,
+ changeNoteJson,
+ args.metrics,
+ serverId,
+ externalIdCache);
ChangeNotesState state = notesWithComments.parseAll();
assertThat(state.approvals()).isEmpty();
assertThat(state.publishedComments()).hasSize(1);
@@ -2093,7 +2099,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
ChangeNotesParser notesWithApprovals =
new ChangeNotesParser(
- c.getId(), commitWithApprovals.copy(), rw, changeNoteJson, args.metrics);
+ c.getId(),
+ commitWithApprovals.copy(),
+ rw,
+ changeNoteJson,
+ args.metrics,
+ 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
new file mode 100644
index 0000000000..bb49a6de77
--- /dev/null
+++ b/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java
@@ -0,0 +1,114 @@
+// Copyright (C) 2022 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.notedb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+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.inject.AbstractModule;
+import java.util.Optional;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ImportedChangeNotesTest extends AbstractChangeNotesTest {
+
+ private static final String FOREIGN_SERVER_ID = "foreign-server-id";
+ private static final String IMPORTED_SERVER_ID = "gerrit-imported-1";
+
+ private ExternalIdCache externalIdCacheMock;
+
+ @Before
+ @Override
+ public void setUpTestEnvironment() throws Exception {
+ setupTestPrerequisites();
+ }
+
+ private void initServerIds(String serverId, String... importedServerIds)
+ throws Exception, RepositoryCaseMismatchException, RepositoryNotFoundException {
+ externalIdCacheMock = mock(ExternalIdCache.class);
+ injector =
+ createTestInjector(
+ new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(ExternalIdCache.class).toInstance(externalIdCacheMock);
+ }
+ },
+ serverId,
+ importedServerIds);
+ injector.injectMembers(this);
+ createAllUsers(injector);
+ }
+
+ @Test
+ 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));
+
+ Change importedChange = newChange(createTestInjector(IMPORTED_SERVER_ID), false);
+ Change localChange = newChange();
+
+ assertThat(newNotes(importedChange).getServerId()).isEqualTo(IMPORTED_SERVER_ID);
+ assertThat(newNotes(localChange).getServerId()).isEqualTo(LOCAL_SERVER_ID);
+ }
+
+ @Test
+ public void rejectChangeWithForeignServerId() throws Exception {
+ initServerIds(LOCAL_SERVER_ID);
+ when(externalIdCacheMock.byKey(any())).thenReturn(Optional.empty());
+
+ Change foreignChange = newChange(createTestInjector(FOREIGN_SERVER_ID), false);
+
+ InvalidServerIdException invalidServerIdEx =
+ assertThrows(InvalidServerIdException.class, () -> newNotes(foreignChange));
+
+ String invalidServerIdMessage = invalidServerIdEx.getMessage();
+ assertThat(invalidServerIdMessage).contains("expected " + LOCAL_SERVER_ID);
+ assertThat(invalidServerIdMessage).contains("actual: " + FOREIGN_SERVER_ID);
+ }
+
+ @Test
+ public void changeFromImportedServerIdWithUnknownAccountId() throws Exception {
+ initServerIds(LOCAL_SERVER_ID, IMPORTED_SERVER_ID);
+
+ when(externalIdCacheMock.byKey(any())).thenReturn(Optional.empty());
+
+ Change importedChange = newChange(createTestInjector(IMPORTED_SERVER_ID), false);
+ assertThat(newNotes(importedChange).getServerId()).isEqualTo(IMPORTED_SERVER_ID);
+
+ assertThat(newNotes(importedChange).getChange().getOwner())
+ .isEqualTo(Account.UNKNOWN_ACCOUNT_ID);
+ }
+}