Add ExternalIdUpsertPreprocessor interface
By default this is not bound and behavior is unchanged. If bound,
ExternalIdNotes calls this before external IDs are upserted. The
preprocessor can do work related to the linked account. It can also
block the operation by throwing an exception.
Change-Id: I7ed895b777e3fc3aca5555729b11cf56d9eb3e32
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
index 50a2f69..435a524 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
@@ -29,6 +29,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
@@ -97,12 +98,17 @@
protected final ExternalIdCache externalIdCache;
protected final MetricMaker metricMaker;
protected final AllUsersName allUsersName;
+ protected final DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors;
protected ExternalIdNotesLoader(
- ExternalIdCache externalIdCache, MetricMaker metricMaker, AllUsersName allUsersName) {
+ ExternalIdCache externalIdCache,
+ MetricMaker metricMaker,
+ AllUsersName allUsersName,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
this.externalIdCache = externalIdCache;
this.metricMaker = metricMaker;
this.allUsersName = allUsersName;
+ this.upsertPreprocessors = upsertPreprocessors;
}
/**
@@ -192,21 +198,24 @@
ExternalIdCache externalIdCache,
Provider<AccountIndexer> accountIndexer,
MetricMaker metricMaker,
- AllUsersName allUsersName) {
- super(externalIdCache, metricMaker, allUsersName);
+ AllUsersName allUsersName,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
+ super(externalIdCache, metricMaker, allUsersName, upsertPreprocessors);
this.accountIndexer = accountIndexer;
}
@Override
public ExternalIdNotes load(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).load();
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .load();
}
@Override
public ExternalIdNotes load(Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).load(rev);
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .load(rev);
}
@Override
@@ -220,20 +229,27 @@
@Inject
FactoryNoReindex(
- ExternalIdCache externalIdCache, MetricMaker metricMaker, AllUsersName allUsersName) {
- super(externalIdCache, metricMaker, allUsersName);
+ ExternalIdCache externalIdCache,
+ MetricMaker metricMaker,
+ AllUsersName allUsersName,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
+ super(externalIdCache, metricMaker, allUsersName, upsertPreprocessors);
}
@Override
public ExternalIdNotes load(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).setNoReindex().load();
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .setNoReindex()
+ .load();
}
@Override
public ExternalIdNotes load(Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).setNoReindex().load(rev);
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .setNoReindex()
+ .load(rev);
}
@Override
@@ -255,7 +271,8 @@
public static ExternalIdNotes loadReadOnly(
AllUsersName allUsersName, Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(new DisabledMetricMaker(), allUsersName, allUsersRepo)
+ return new ExternalIdNotes(
+ new DisabledMetricMaker(), allUsersName, allUsersRepo, DynamicMap.emptyMap())
.setReadOnly()
.setNoCacheUpdate()
.setNoReindex()
@@ -275,7 +292,8 @@
public static ExternalIdNotes loadNoCacheUpdate(
AllUsersName allUsersName, Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(new DisabledMetricMaker(), allUsersName, allUsersRepo)
+ return new ExternalIdNotes(
+ new DisabledMetricMaker(), allUsersName, allUsersRepo, DynamicMap.emptyMap())
.setNoCacheUpdate()
.setNoReindex()
.load();
@@ -284,6 +302,7 @@
private final AllUsersName allUsersName;
private final Counter0 updateCount;
private final Repository repo;
+ private final DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors;
private final CallerFinder callerFinder;
private NoteMap noteMap;
@@ -312,13 +331,17 @@
private boolean noReindex = false;
private ExternalIdNotes(
- MetricMaker metricMaker, AllUsersName allUsersName, Repository allUsersRepo) {
+ MetricMaker metricMaker,
+ AllUsersName allUsersName,
+ Repository allUsersRepo,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
this.updateCount =
metricMaker.newCounter(
"notedb/external_id_update_count",
new Description("Total number of external ID updates.").setRate().setUnit("updates"));
this.allUsersName = requireNonNull(allUsersName, "allUsersRepo");
this.repo = requireNonNull(allUsersRepo, "allUsersRepo");
+ this.upsertPreprocessors = upsertPreprocessors;
this.callerFinder =
CallerFinder.builder()
// 1. callers that come through ExternalIds
@@ -490,6 +513,7 @@
(rw, n) -> {
for (ExternalId extId : extIds) {
ExternalId insertedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(insertedExtId);
newExtIds.add(insertedExtId);
}
});
@@ -519,6 +543,7 @@
(rw, n) -> {
for (ExternalId extId : extIds) {
ExternalId updatedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(updatedExtId);
updatedExtIds.add(updatedExtId);
}
});
@@ -634,6 +659,7 @@
for (ExternalId extId : toAdd) {
ExternalId insertedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(insertedExtId);
updatedExtIds.add(insertedExtId);
}
});
@@ -667,6 +693,7 @@
for (ExternalId extId : toAdd) {
ExternalId insertedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(insertedExtId);
updatedExtIds.add(insertedExtId);
}
});
@@ -809,9 +836,9 @@
}
/**
- * Insert or updates an new external ID and sets it in the note map.
+ * Inserts or updates a new external ID and sets it in the note map.
*
- * <p>If the external ID already exists it is overwritten.
+ * <p>If the external ID already exists, it is overwritten.
*/
private static ExternalId upsert(
RevWalk rw, ObjectInserter ins, NoteMap noteMap, ExternalId extId)
@@ -867,6 +894,7 @@
* @return the external ID that was removed, {@code null} if no external ID with the specified key
* exists
*/
+ @Nullable
private static ExternalId remove(
RevWalk rw, NoteMap noteMap, ExternalId.Key extIdKey, Account.Id expectedAccountId)
throws IOException, ConfigInvalidException {
@@ -917,6 +945,10 @@
checkState(noteMap != null, "External IDs not loaded yet");
}
+ private void preprocessUpsert(ExternalId extId) {
+ upsertPreprocessors.forEach(p -> p.get().upsert(extId));
+ }
+
@FunctionalInterface
private interface NoteMapUpdate {
void execute(RevWalk rw, NoteMap noteMap)
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdUpsertPreprocessor.java b/java/com/google/gerrit/server/account/externalids/ExternalIdUpsertPreprocessor.java
new file mode 100644
index 0000000..c0697db
--- /dev/null
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdUpsertPreprocessor.java
@@ -0,0 +1,30 @@
+// Copyright (C) 2021 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.account.externalids;
+
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+
+/**
+ * This optional preprocessor is called in {@link ExternalIdNotes} before an update is committed.
+ */
+@ExtensionPoint
+public interface ExternalIdUpsertPreprocessor {
+ /**
+ * Called when inserting or updating an external ID. {@link ExternalId#blobId()} is set. The
+ * upsert can be blocked by throwing {@link com.google.gerrit.exceptions.StorageException}, e.g.
+ * when a precondition or preparatory work fails.
+ */
+ void upsert(ExternalId extId);
+}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 076ba46..122aa26 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -100,6 +100,7 @@
import com.google.gerrit.server.account.ServiceUserClassifierImpl;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalIdModule;
+import com.google.gerrit.server.account.externalids.ExternalIdUpsertPreprocessor;
import com.google.gerrit.server.approval.ApprovalCacheImpl;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.auth.AuthBackend;
@@ -476,5 +477,6 @@
bind(ReloadPluginListener.class)
.annotatedWith(UniqueAnnotations.create())
.to(PluginConfigFactory.class);
+ DynamicMap.mapOf(binder(), ExternalIdUpsertPreprocessor.class);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java
new file mode 100644
index 0000000..64ad900
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java
@@ -0,0 +1,209 @@
+// Copyright (C) 2021 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.acceptance.server.notedb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.Accounts;
+import com.google.gerrit.server.account.AccountsUpdate;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdNotes;
+import com.google.gerrit.server.account.externalids.ExternalIdUpsertPreprocessor;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.notedb.Sequences;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.util.ArrayList;
+import java.util.List;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests {@link ExternalIdUpsertPreprocessor}. */
+@TestPlugin(
+ name = "external-id-update-preprocessor",
+ sysModule =
+ "com.google.gerrit.acceptance.server.notedb.ExternalIdNotesUpsertPreprocessorIT$Module")
+public class ExternalIdNotesUpsertPreprocessorIT extends LightweightPluginDaemonTest {
+ @Inject private Sequences sequences;
+ @Inject private @ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider;
+ @Inject private ExternalIdNotes.Factory extIdNotesFactory;
+ @Inject private Accounts accounts;
+
+ public static class Module extends AbstractModule {
+ @Override
+ protected void configure() {
+ bind(ExternalIdUpsertPreprocessor.class)
+ .annotatedWith(Exports.named("TestPreprocessor"))
+ .to(TestPreprocessor.class);
+ }
+ }
+
+ private TestPreprocessor testPreprocessor;
+
+ @Before
+ public void setUp() {
+ testPreprocessor = plugin.getSysInjector().getInstance(TestPreprocessor.class);
+ testPreprocessor.reset();
+ }
+
+ @Test
+ public void insertAccount() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId));
+ assertThat(testPreprocessor.upserted).containsExactly(extId);
+ }
+
+ @Test
+ public void replaceByKeys() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar1", id);
+ ExternalId extId2 = ExternalId.create("foo", "bar2", id);
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ testPreprocessor.reset();
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.replaceByKeys(ImmutableSet.of(extId1.key()), ImmutableSet.of(extId2));
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId2);
+ }
+
+ @Test
+ public void insert() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.insert(extId);
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId);
+ }
+
+ @Test
+ public void upsert() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.upsert(extId);
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId);
+ }
+
+ @Test
+ public void replace() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar1", id);
+ ExternalId extId2 = ExternalId.create("foo", "bar2", id);
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ testPreprocessor.reset();
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.replace(ImmutableSet.of(extId1), ImmutableSet.of(extId2));
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId2);
+ }
+
+ @Test
+ public void replace_viaAccountsUpdate() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar", id, "email1@foo", "hash");
+ ExternalId extId2 = ExternalId.create("foo", "bar", id, "email2@foo", "hash");
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ testPreprocessor.reset();
+ accountsUpdateProvider.get().update("test", id, u -> u.updateExternalId(extId2));
+ assertThat(testPreprocessor.upserted).containsExactly(extId2);
+ }
+
+ @Test
+ public void blockUpsert() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+ testPreprocessor.throwException = true;
+ StorageException e =
+ assertThrows(
+ StorageException.class,
+ () -> accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId)));
+ assertThat(e).hasMessageThat().contains("upsert not good");
+ assertThat(testPreprocessor.upserted).isEmpty();
+ }
+
+ @Test
+ public void blockUpsert_replace() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar", id, "email1@foo", "hash");
+ ExternalId extId2 = ExternalId.create("foo", "bar", id, "email2@foo", "hash");
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ assertThat(accounts.get(id).get().externalIds()).containsExactly(extId1);
+
+ testPreprocessor.reset();
+ testPreprocessor.throwException = true;
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.replace(ImmutableSet.of(extId1), ImmutableSet.of(extId2));
+ StorageException e = assertThrows(StorageException.class, () -> extIdNotes.commit(md));
+ assertThat(e).hasMessageThat().contains("upsert not good");
+ }
+ assertThat(testPreprocessor.upserted).isEmpty();
+ assertThat(accounts.get(id).get().externalIds()).containsExactly(extId1);
+ }
+
+ @Singleton
+ public static class TestPreprocessor implements ExternalIdUpsertPreprocessor {
+ List<ExternalId> upserted = new ArrayList<>();
+
+ boolean throwException = false;
+
+ @Override
+ public void upsert(ExternalId extId) {
+ assertThat(extId.blobId()).isNotNull();
+ if (throwException) {
+ throw new StorageException("upsert not good");
+ }
+ upserted.add(extId);
+ }
+
+ void reset() {
+ upserted.clear();
+ throwException = false;
+ }
+ }
+}
