diff --git a/java/com/google/gerrit/server/group/db/AuditLogReader.java b/java/com/google/gerrit/server/group/db/AuditLogReader.java index fb58577f8d..77e0e0ce98 100644 --- a/java/com/google/gerrit/server/group/db/AuditLogReader.java +++ b/java/com/google/gerrit/server/group/db/AuditLogReader.java @@ -27,7 +27,6 @@ import com.google.gerrit.reviewdb.client.AccountGroupByIdAudit; import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.config.GerritServerId; import com.google.gerrit.server.notedb.NoteDbUtil; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -51,12 +50,10 @@ import org.eclipse.jgit.util.RawParseUtils; public class AuditLogReader { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private final String serverId; private final AllUsersName allUsersName; @Inject - public AuditLogReader(@GerritServerId String serverId, AllUsersName allUsersName) { - this.serverId = serverId; + public AuditLogReader(AllUsersName allUsersName) { this.allUsersName = allUsersName; } @@ -143,7 +140,7 @@ public class AuditLogReader { } private Optional parse(AccountGroup.UUID uuid, RevCommit c) { - Optional authorId = NoteDbUtil.parseIdent(c.getAuthorIdent(), serverId); + Optional 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. @@ -179,7 +176,7 @@ public class AuditLogReader { private Optional parseAccount(AccountGroup.UUID uuid, RevCommit c, FooterLine line) { Optional result = Optional.ofNullable(RawParseUtils.parsePersonIdent(line.getValue())) - .flatMap(ident -> NoteDbUtil.parseIdent(ident, serverId)); + .flatMap(ident -> NoteDbUtil.parseIdent(ident)); if (!result.isPresent()) { logInvalid(uuid, c, line); } diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java index 9cc1c84e25..10bae48730 100644 --- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java @@ -25,6 +25,7 @@ import com.google.gerrit.metrics.Timer1; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.GerritServerId; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk; import com.google.gerrit.server.project.NoSuchChangeException; @@ -51,6 +52,7 @@ public abstract class AbstractChangeNotes { public final AllUsersName allUsers; public final LegacyChangeNoteRead legacyChangeNoteRead; public final NoteDbMetrics metrics; + public final String serverId; // Providers required to avoid dependency cycles. @@ -64,7 +66,8 @@ public abstract class AbstractChangeNotes { ChangeNoteJson changeNoteJson, LegacyChangeNoteRead legacyChangeNoteRead, NoteDbMetrics metrics, - Provider cache) { + Provider cache, + @GerritServerId String serverId) { this.failOnLoadForTest = new AtomicBoolean(); this.repoManager = repoManager; this.allUsers = allUsers; @@ -72,6 +75,7 @@ public abstract class AbstractChangeNotes { this.changeNoteJson = changeNoteJson; this.metrics = metrics; this.cache = cache; + this.serverId = serverId; } } diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java index e1217c2e59..929974dec3 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -22,6 +22,7 @@ import static java.util.Objects.requireNonNull; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; @@ -503,6 +504,19 @@ public class ChangeNotes extends AbstractChangeNotes { ChangeNotesCache.Value v = args.cache.get().get(getProjectName(), getChangeId(), rev, handle::walk); state = v.state(); + + String stateServerId = state.serverId(); + /** + * In earlier Gerrit versions serverId wasn't part of the change notes cache. That's why the + * earlier cached entries don't have the serverId attribute. That's fine because in earlier + * gerrit version serverId was already validated. Another approach to simplify the check would + * be to bump the cache version, but that would invalidate all persistent cache entries, what we + * rather try to avoid. + */ + checkState( + Strings.isNullOrEmpty(stateServerId) || args.serverId.equals(stateServerId), + String.format("invalid server id, expected %s: actual: %s", args.serverId, stateServerId)); + state.copyColumnsTo(change); revisionNoteMap = v.revisionNoteMap(); } diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 27c2aa621c..b5087ffc18 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -135,6 +135,7 @@ class ChangeNotesParser { private Timestamp createdOn; private Timestamp lastUpdatedOn; private Account.Id ownerId; + private String serverId; private String changeId; private String subject; private String originalSubject; @@ -223,6 +224,7 @@ class ChangeNotesParser { createdOn, lastUpdatedOn, ownerId, + serverId, branch, buildCurrentPatchSetId(), subject, @@ -334,6 +336,10 @@ class ChangeNotesParser { Account.Id accountId = parseIdent(commit); if (accountId != null) { ownerId = accountId; + PersonIdent personIdent = commit.getAuthorIdent(); + serverId = NoteDbUtil.extractHostPartFromPersonIdent(personIdent); + } else { + serverId = "UNKNOWN_SERVER_ID"; } Account.Id realAccountId = parseRealAccountId(commit, accountId); diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java index 27285161cc..5adc196fac 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java @@ -96,6 +96,7 @@ public abstract class ChangeNotesState { Timestamp createdOn, Timestamp lastUpdatedOn, Account.Id owner, + String serverId, String branch, @Nullable PatchSet.Id currentPatchSetId, String subject, @@ -154,6 +155,7 @@ public abstract class ChangeNotesState { .build()) .pastAssignees(pastAssignees) .hashtags(hashtags) + .serverId(serverId) .patchSets(patchSets.entrySet()) .approvals(approvals.entries()) .reviewers(reviewers) @@ -276,6 +278,9 @@ public abstract class ChangeNotesState { abstract ImmutableSet hashtags(); + @Nullable + abstract String serverId(); + abstract ImmutableList> patchSets(); abstract ImmutableList> approvals(); @@ -376,6 +381,8 @@ public abstract class ChangeNotesState { abstract Builder columns(ChangeColumns columns); + abstract Builder serverId(String serverId); + abstract Builder pastAssignees(Set pastAssignees); abstract Builder hashtags(Iterable hashtags); @@ -427,6 +434,10 @@ public abstract class ChangeNotesState { .setChangeId(object.changeId().get()) .setColumns(toChangeColumnsProto(object.columns())); + if (object.serverId() != null) { + b.setServerId(object.serverId()); + b.setHasServerId(true); + } object.pastAssignees().forEach(a -> b.addPastAssignee(a.get())); object.hashtags().forEach(b::addHashtag); object @@ -549,6 +560,7 @@ public abstract class ChangeNotesState { .metaId(ObjectIdConverter.create().fromByteString(proto.getMetaId())) .changeId(changeId) .columns(toChangeColumns(changeId, proto.getColumns())) + .serverId(proto.getHasServerId() ? proto.getServerId() : null) .pastAssignees( proto.getPastAssigneeList().stream().map(Account::id).collect(toImmutableSet())) .hashtags(proto.getHashtagList()) diff --git a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java index 36bfe4765e..7e301f725a 100644 --- a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java +++ b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java @@ -50,14 +50,11 @@ public class LegacyChangeNoteRead { public Account.Id parseIdent(PersonIdent ident, Change.Id changeId) throws ConfigInvalidException { - return NoteDbUtil.parseIdent(ident, serverId) + return NoteDbUtil.parseIdent(ident) .orElseThrow( () -> parseException( - changeId, - "invalid identity, expected @%s: %s", - serverId, - ident.getEmailAddress())); + changeId, "cannot retrieve account id: %s", ident.getEmailAddress())); } private static boolean match(byte[] note, MutableInteger p, byte[] expected) { diff --git a/java/com/google/gerrit/server/notedb/NoteDbUtil.java b/java/com/google/gerrit/server/notedb/NoteDbUtil.java index c53f4b9880..bfbdd18c31 100644 --- a/java/com/google/gerrit/server/notedb/NoteDbUtil.java +++ b/java/com/google/gerrit/server/notedb/NoteDbUtil.java @@ -37,25 +37,28 @@ public class NoteDbUtil { ImmutableSet.of( "com.google.gerrit.httpd.restapi.RestApiServlet", RetryingRestModifyView.class.getName()); - /** - * Returns an AccountId for the given email address. Returns empty if the address isn't on this - * server. - */ - public static Optional parseIdent(PersonIdent ident, String serverId) { + /** Returns an AccountId for the given email address. */ + public static Optional parseIdent(PersonIdent ident) { String email = ident.getEmailAddress(); int at = email.indexOf('@'); if (at >= 0) { - String host = email.substring(at + 1); - if (host.equals(serverId)) { - Integer id = Ints.tryParse(email.substring(0, at)); - if (id != null) { - return Optional.of(Account.id(id)); - } + Integer id = Ints.tryParse(email.substring(0, at)); + if (id != null) { + return Optional.of(Account.id(id)); } } return Optional.empty(); } + public static String extractHostPartFromPersonIdent(PersonIdent ident) { + String email = ident.getEmailAddress(); + int at = email.indexOf('@'); + if (at >= 0) { + return email.substring(at + 1); + } + throw new IllegalArgumentException("No host part found: " + email); + } + public static String formatTime(PersonIdent ident, Timestamp t) { GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT); // TODO(dborowitz): Use a ThreadLocal or use Joda. diff --git a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java index a5b755e3aa..81108ea565 100644 --- a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java +++ b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java @@ -37,7 +37,7 @@ public final class AuditLogReaderTest extends AbstractGroupTest { @Before public void setUp() throws Exception { - auditLogReader = new AuditLogReader(SERVER_ID, allUsersName); + auditLogReader = new AuditLogReader(allUsersName); } @Test diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java index 3e54863f31..a4602e19b0 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java @@ -56,6 +56,7 @@ import java.sql.Timestamp; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.UUID; import org.eclipse.jgit.lib.ObjectId; import org.junit.Before; import org.junit.Test; @@ -66,6 +67,7 @@ public class ChangeNotesStateTest { ObjectId.fromString("1234567812345678123456781234567812345678"); private static final ByteString SHA_BYTES = ObjectIdConverter.create().toByteString(SHA); private static final String CHANGE_KEY = "Iabcdabcdabcdabcdabcdabcdabcdabcdabcdabcd"; + private static final String DEFAULT_SERVER_ID = UUID.randomUUID().toString(); private ChangeColumns cols; private ChangeColumnsProto colsProto; @@ -717,6 +719,7 @@ public class ChangeNotesStateTest { ImmutableMap.builder() .put("metaId", ObjectId.class) .put("changeId", Change.Id.class) + .put("serverId", String.class) .put("columns", ChangeColumns.class) .put("pastAssignees", new TypeLiteral>() {}.getType()) .put("hashtags", new TypeLiteral>() {}.getType()) @@ -918,6 +921,19 @@ public class ChangeNotesStateTest { .build()); } + @Test + public void serializeServerId() throws Exception { + assertRoundTrip( + newBuilder().serverId(DEFAULT_SERVER_ID).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setServerId(DEFAULT_SERVER_ID) + .setHasServerId(true) + .setColumns(colsProto.toBuilder()) + .build()); + } + private static ChangeNotesStateProto toProto(ChangeNotesState state) throws Exception { return ChangeNotesStateProto.parseFrom(Serializer.INSTANCE.serialize(state)); } diff --git a/proto/cache.proto b/proto/cache.proto index 77b6908b0d..7e6abccfc0 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -186,6 +186,9 @@ message ChangeNotesStateProto { // Number of updates to the change's meta ref. int32 update_count = 19; + + string server_id = 20; + bool has_server_id = 21; }