From 2839aa67b192cb80fb2cc4afa7fca07f8162c173 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 13 Dec 2018 14:46:03 -0800 Subject: [PATCH] Remove NoteDbChangeState This was used for coordinating the state of migrating a single change to NoteDb, so is no longer required. Change-Id: Ie5c313140d810ded904d34273d6118fba2866516 --- .../google/gerrit/reviewdb/client/Change.java | 13 - .../gerrit/server/notedb/ChangeNotes.java | 8 +- .../server/notedb/ChangeNotesState.java | 26 -- .../server/notedb/NoteDbChangeState.java | 441 ------------------ .../server/notedb/NoteDbChangeStateTest.java | 241 ---------- proto/reviewdb.proto | 12 +- 6 files changed, 8 insertions(+), 733 deletions(-) delete mode 100644 java/com/google/gerrit/server/notedb/NoteDbChangeState.java delete mode 100644 javatests/com/google/gerrit/server/notedb/NoteDbChangeStateTest.java diff --git a/java/com/google/gerrit/reviewdb/client/Change.java b/java/com/google/gerrit/reviewdb/client/Change.java index 8d4de0526f..262b9e04c9 100644 --- a/java/com/google/gerrit/reviewdb/client/Change.java +++ b/java/com/google/gerrit/reviewdb/client/Change.java @@ -520,10 +520,6 @@ public final class Change { @Column(id = 23, notNull = false) protected Id revertOf; - /** @see com.google.gerrit.server.notedb.NoteDbChangeState */ - @Column(id = 101, notNull = false, length = Integer.MAX_VALUE) - protected String noteDbState; - protected Change() {} public Change( @@ -559,7 +555,6 @@ public final class Change { isPrivate = other.isPrivate; workInProgress = other.workInProgress; reviewStarted = other.reviewStarted; - noteDbState = other.noteDbState; revertOf = other.revertOf; } @@ -738,14 +733,6 @@ public final class Change { return this.revertOf; } - public String getNoteDbState() { - return noteDbState; - } - - public void setNoteDbState(String state) { - noteDbState = state; - } - @Override public String toString() { return new StringBuilder(getClass().getSimpleName()) diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java index 2717488705..182af53dee 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -109,7 +109,6 @@ public class ChangeNotes extends AbstractChangeNotes { public ChangeNotes createChecked(Project.NameKey project, Change.Id changeId) throws OrmException { - // Prepopulate the change exists with proper noteDbState field. Change change = newChange(project, changeId); return new ChangeNotes(args, change, true, null).load(); } @@ -128,11 +127,8 @@ public class ChangeNotes extends AbstractChangeNotes { } public static Change newChange(Project.NameKey project, Change.Id changeId) { - Change change = - new Change( - null, changeId, null, new Branch.NameKey(project, "INVALID_NOTE_DB_ONLY"), null); - change.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); - return change; + return new Change( + null, changeId, null, new Branch.NameKey(project, "INVALID_NOTE_DB_ONLY"), null); } public ChangeNotes create(Project.NameKey project, Change.Id changeId) throws OrmException { diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java index 52cd99f2e0..9859f107bd 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java @@ -62,7 +62,6 @@ import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.Reviewer import com.google.gerrit.server.cache.serialize.CacheSerializer; import com.google.gerrit.server.cache.serialize.ObjectIdConverter; import com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord; -import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gson.Gson; import com.google.protobuf.ByteString; import com.google.protobuf.MessageLite; @@ -173,9 +172,6 @@ public abstract class ChangeNotesState { /** * Subset of Change columns that can be represented in NoteDb. * - *

Notable exceptions include rowVersion and noteDbState, which are only make sense when read - * from NoteDb, so they cannot be cached. - * *

Fields should match the column names in {@link Change}, and are in listed column order. */ @AutoValue @@ -312,7 +308,6 @@ public abstract class ChangeNotesState { new Branch.NameKey(project, c.branch()), c.createdOn()); copyNonConstructorColumnsTo(change); - change.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); return change; } @@ -322,7 +317,6 @@ public abstract class ChangeNotesState { c != null && metaId() != null, "missing columns or metaId in ChangeNotesState; is NoteDb enabled? %s", this); - checkMetaId(change); change.setKey(c.changeKey()); change.setOwner(c.owner()); change.setDest(new Branch.NameKey(change.getProject(), c.branch())); @@ -330,26 +324,6 @@ public abstract class ChangeNotesState { copyNonConstructorColumnsTo(change); } - private void checkMetaId(Change change) throws IOException { - NoteDbChangeState state = NoteDbChangeState.parse(change); - if (state == null) { - return; // Can happen during small NoteDb tests. - } else if (state.getPrimaryStorage() == PrimaryStorage.NOTE_DB) { - return; - } - checkState(state.getRefState().isPresent(), "expected RefState: %s", state); - ObjectId idFromState = state.getRefState().get().changeMetaId(); - if (!idFromState.equals(metaId())) { - throw new IOException( - "cannot copy ChangeNotesState into Change " - + changeId() - + "; this ChangeNotesState was created from " - + metaId() - + ", but change requires state " - + idFromState); - } - } - private void copyNonConstructorColumnsTo(Change change) { ChangeColumns c = requireNonNull(columns(), "columns are required"); if (c.status() != null) { diff --git a/java/com/google/gerrit/server/notedb/NoteDbChangeState.java b/java/com/google/gerrit/server/notedb/NoteDbChangeState.java deleted file mode 100644 index d63994eb0b..0000000000 --- a/java/com/google/gerrit/server/notedb/NoteDbChangeState.java +++ /dev/null @@ -1,441 +0,0 @@ -// Copyright (C) 2016 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.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkState; -import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef; -import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments; -import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.NOTE_DB; -import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; -import static java.util.Objects.requireNonNull; - -import com.google.auto.value.AutoValue; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Splitter; -import com.google.common.base.Strings; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; -import com.google.common.primitives.Longs; -import com.google.gerrit.common.Nullable; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.server.ReviewDbUtil; -import com.google.gerrit.server.git.RefCache; -import com.google.gerrit.server.util.time.TimeUtil; -import com.google.gwtorm.server.OrmRuntimeException; -import java.io.IOException; -import java.sql.Timestamp; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.concurrent.TimeUnit; -import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.ObjectId; - -/** - * The state of all relevant NoteDb refs across all repos corresponding to a given Change entity. - * - *

Stored serialized in the {@code Change#noteDbState} field, and used to determine whether the - * state in NoteDb is out of date. - * - *

Serialized in one of the forms: - * - *

    - *
  • [meta-sha],[account1]=[drafts-sha],[account2]=[drafts-sha]... - *
  • R,[meta-sha],[account1]=[drafts-sha],[account2]=[drafts-sha]... - *
  • R=[read-only-until],[meta-sha],[account1]=[drafts-sha],[account2]=[drafts-sha]... - *
  • N - *
  • N=[read-only-until] - *
- * - * in numeric account ID order, with hex SHA-1s for human readability. - */ -public class NoteDbChangeState { - public static final String NOTE_DB_PRIMARY_STATE = "N"; - - public enum PrimaryStorage { - REVIEW_DB('R'), - NOTE_DB('N'); - - private final char code; - - PrimaryStorage(char code) { - this.code = code; - } - - public static PrimaryStorage of(@Nullable Change c) { - NoteDbChangeState s = NoteDbChangeState.parse(c); - return s != null ? s.getPrimaryStorage() : REVIEW_DB; - } - } - - @AutoValue - public abstract static class Delta { - @VisibleForTesting - public static Delta create( - Change.Id changeId, - Optional newChangeMetaId, - Map newDraftIds) { - if (newDraftIds == null) { - newDraftIds = ImmutableMap.of(); - } - return new AutoValue_NoteDbChangeState_Delta( - changeId, newChangeMetaId, ImmutableMap.copyOf(newDraftIds)); - } - - abstract Change.Id changeId(); - - abstract Optional newChangeMetaId(); - - abstract ImmutableMap newDraftIds(); - } - - @AutoValue - public abstract static class RefState { - @VisibleForTesting - public static RefState create(ObjectId changeMetaId, Map draftIds) { - return new AutoValue_NoteDbChangeState_RefState( - changeMetaId.copy(), - ImmutableMap.copyOf(Maps.filterValues(draftIds, id -> !ObjectId.zeroId().equals(id)))); - } - - private static Optional parse(Change.Id changeId, List parts) { - checkArgument(!parts.isEmpty(), "missing state string for change %s", changeId); - ObjectId changeMetaId = ObjectId.fromString(parts.get(0)); - Map draftIds = Maps.newHashMapWithExpectedSize(parts.size() - 1); - Splitter s = Splitter.on('='); - for (int i = 1; i < parts.size(); i++) { - String p = parts.get(i); - List draftParts = s.splitToList(p); - checkArgument( - draftParts.size() == 2, "invalid draft state part for change %s: %s", changeId, p); - Optional accountId = Account.Id.tryParse(draftParts.get(0)); - checkArgument( - accountId.isPresent(), - "invalid account ID in draft state part for change %s: %s", - changeId, - p); - draftIds.put(accountId.get(), ObjectId.fromString(draftParts.get(1))); - } - return Optional.of(create(changeMetaId, draftIds)); - } - - abstract ObjectId changeMetaId(); - - abstract ImmutableMap draftIds(); - - @Override - public String toString() { - return appendTo(new StringBuilder()).toString(); - } - - StringBuilder appendTo(StringBuilder sb) { - sb.append(changeMetaId().name()); - for (Account.Id id : ReviewDbUtil.intKeyOrdering().sortedCopy(draftIds().keySet())) { - sb.append(',').append(id.get()).append('=').append(draftIds().get(id).name()); - } - return sb; - } - } - - public static NoteDbChangeState parse(@Nullable Change c) { - return c != null ? parse(c.getId(), c.getNoteDbState()) : null; - } - - @VisibleForTesting - public static NoteDbChangeState parse(Change.Id id, @Nullable String str) { - if (Strings.isNullOrEmpty(str)) { - // Return null rather than Optional as this is what goes in the field in - // ReviewDb. - return null; - } - List parts = Splitter.on(',').splitToList(str); - String first = parts.get(0); - Optional readOnlyUntil = parseReadOnlyUntil(id, str, first); - - // Only valid NOTE_DB state is "N". - if (parts.size() == 1 && first.charAt(0) == NOTE_DB.code) { - return new NoteDbChangeState(id, NOTE_DB, Optional.empty(), readOnlyUntil); - } - - // Otherwise it must be REVIEW_DB, either "R," or just - // "". Allow length > 0 for forward compatibility. - if (first.length() > 0) { - Optional refState; - if (first.charAt(0) == REVIEW_DB.code) { - refState = RefState.parse(id, parts.subList(1, parts.size())); - } else { - refState = RefState.parse(id, parts); - } - return new NoteDbChangeState(id, REVIEW_DB, refState, readOnlyUntil); - } - throw invalidState(id, str); - } - - private static Optional parseReadOnlyUntil( - Change.Id id, String fullStr, String first) { - if (first.length() > 2 && first.charAt(1) == '=') { - Long ts = Longs.tryParse(first.substring(2)); - if (ts == null) { - throw invalidState(id, fullStr); - } - return Optional.of(new Timestamp(ts)); - } - return Optional.empty(); - } - - private static IllegalArgumentException invalidState(Change.Id id, String str) { - return new IllegalArgumentException("invalid state string for change " + id + ": " + str); - } - - /** - * Apply a delta to the state stored in a change entity. - * - *

This method does not check whether the old state was read-only; it is up to the caller to - * not violate read-only semantics when storing the change back in ReviewDb. - * - * @param change change entity. The delta is applied against this entity's {@code noteDbState} and - * the new state is stored back in the entity as a side effect. - * @param delta delta to apply. - * @return new state, equivalent to what is stored in {@code change} as a side effect. - */ - public static NoteDbChangeState applyDelta(Change change, Delta delta) { - if (delta == null) { - return null; - } - String oldStr = change.getNoteDbState(); - if (oldStr == null && !delta.newChangeMetaId().isPresent()) { - // Neither an old nor a new meta ID was present, most likely because we - // aren't writing a NoteDb graph at all for this change at this point. No - // point in proceeding. - return null; - } - NoteDbChangeState oldState = parse(change.getId(), oldStr); - if (oldState != null && oldState.getPrimaryStorage() == NOTE_DB) { - // NOTE_DB state doesn't include RefState, so applying a delta is a no-op. - return oldState; - } - - ObjectId changeMetaId; - if (delta.newChangeMetaId().isPresent()) { - changeMetaId = delta.newChangeMetaId().get(); - if (changeMetaId.equals(ObjectId.zeroId())) { - change.setNoteDbState(null); - return null; - } - } else { - changeMetaId = oldState.getChangeMetaId(); - } - - Map draftIds = new HashMap<>(); - if (oldState != null) { - draftIds.putAll(oldState.getDraftIds()); - } - for (Map.Entry e : delta.newDraftIds().entrySet()) { - if (e.getValue().equals(ObjectId.zeroId())) { - draftIds.remove(e.getKey()); - } else { - draftIds.put(e.getKey(), e.getValue()); - } - } - - NoteDbChangeState state = - new NoteDbChangeState( - change.getId(), - oldState != null ? oldState.getPrimaryStorage() : REVIEW_DB, - Optional.of(RefState.create(changeMetaId, draftIds)), - // Copy old read-only deadline rather than advancing it; the caller is - // still responsible for finishing the rest of its work before the lease - // runs out. - oldState != null ? oldState.getReadOnlyUntil() : Optional.empty()); - change.setNoteDbState(state.toString()); - return state; - } - - public static long getReadOnlySkew(Config cfg) { - return cfg.getTimeUnit("notedb", null, "maxTimestampSkew", 1000, TimeUnit.MILLISECONDS); - } - - static Timestamp timeForReadOnlyCheck(long skewMs) { - // Subtract some slop in case the machine that set the change's read-only - // lease has a clock behind ours. - return new Timestamp(TimeUtil.nowMs() - skewMs); - } - - public static void checkNotReadOnly(@Nullable Change change, long skewMs) { - checkNotReadOnly(parse(change), skewMs); - } - - public static void checkNotReadOnly(@Nullable NoteDbChangeState state, long skewMs) { - if (state == null) { - return; // No state means ReviewDb primary non-read-only. - } else if (state.isReadOnly(timeForReadOnlyCheck(skewMs))) { - throw new OrmRuntimeException( - "change " - + state.getChangeId() - + " is read-only until " - + state.getReadOnlyUntil().get()); - } - } - - private final Change.Id changeId; - private final PrimaryStorage primaryStorage; - private final Optional refState; - private final Optional readOnlyUntil; - - public NoteDbChangeState( - Change.Id changeId, - PrimaryStorage primaryStorage, - Optional refState, - Optional readOnlyUntil) { - this.changeId = requireNonNull(changeId); - this.primaryStorage = requireNonNull(primaryStorage); - this.refState = requireNonNull(refState); - this.readOnlyUntil = requireNonNull(readOnlyUntil); - - switch (primaryStorage) { - case REVIEW_DB: - checkArgument( - refState.isPresent(), - "expected RefState for change %s with primary storage %s", - changeId, - primaryStorage); - break; - case NOTE_DB: - checkArgument( - !refState.isPresent(), - "expected no RefState for change %s with primary storage %s", - changeId, - primaryStorage); - break; - default: - throw new IllegalStateException("invalid PrimaryStorage: " + primaryStorage); - } - } - - public PrimaryStorage getPrimaryStorage() { - return primaryStorage; - } - - public boolean isChangeUpToDate(RefCache changeRepoRefs) throws IOException { - if (primaryStorage == NOTE_DB) { - return true; // Primary storage is NoteDb, up to date by definition. - } - Optional id = changeRepoRefs.get(changeMetaRef(changeId)); - if (!id.isPresent()) { - return getChangeMetaId().equals(ObjectId.zeroId()); - } - return id.get().equals(getChangeMetaId()); - } - - public boolean areDraftsUpToDate(RefCache draftsRepoRefs, Account.Id accountId) - throws IOException { - if (primaryStorage == NOTE_DB) { - return true; // Primary storage is NoteDb, up to date by definition. - } - Optional id = draftsRepoRefs.get(refsDraftComments(changeId, accountId)); - if (!id.isPresent()) { - return !getDraftIds().containsKey(accountId); - } - return id.get().equals(getDraftIds().get(accountId)); - } - - public boolean isUpToDate(RefCache changeRepoRefs, RefCache draftsRepoRefs) throws IOException { - if (primaryStorage == NOTE_DB) { - return true; // Primary storage is NoteDb, up to date by definition. - } - if (!isChangeUpToDate(changeRepoRefs)) { - return false; - } - for (Account.Id accountId : getDraftIds().keySet()) { - if (!areDraftsUpToDate(draftsRepoRefs, accountId)) { - return false; - } - } - return true; - } - - public boolean isReadOnly(Timestamp now) { - return readOnlyUntil.isPresent() && now.before(readOnlyUntil.get()); - } - - public Optional getReadOnlyUntil() { - return readOnlyUntil; - } - - public NoteDbChangeState withReadOnlyUntil(Timestamp ts) { - return new NoteDbChangeState(changeId, primaryStorage, refState, Optional.of(ts)); - } - - public Change.Id getChangeId() { - return changeId; - } - - public ObjectId getChangeMetaId() { - return refState().changeMetaId(); - } - - public ImmutableMap getDraftIds() { - return refState().draftIds(); - } - - public Optional getRefState() { - return refState; - } - - private RefState refState() { - checkState(refState.isPresent(), "state for %s has no RefState: %s", changeId, this); - return refState.get(); - } - - @Override - public String toString() { - switch (primaryStorage) { - case REVIEW_DB: - if (!readOnlyUntil.isPresent()) { - // Don't include enum field, just IDs (though parse would accept it). - return refState().toString(); - } - return primaryStorage.code + "=" + readOnlyUntil.get().getTime() + "," + refState.get(); - case NOTE_DB: - if (!readOnlyUntil.isPresent()) { - return NOTE_DB_PRIMARY_STATE; - } - return primaryStorage.code + "=" + readOnlyUntil.get().getTime(); - default: - throw new IllegalArgumentException("Unsupported PrimaryStorage: " + primaryStorage); - } - } - - @Override - public int hashCode() { - return Objects.hash(changeId, primaryStorage, refState, readOnlyUntil); - } - - @Override - public boolean equals(Object o) { - if (!(o instanceof NoteDbChangeState)) { - return false; - } - NoteDbChangeState s = (NoteDbChangeState) o; - return changeId.equals(s.changeId) - && primaryStorage.equals(s.primaryStorage) - && refState.equals(s.refState) - && readOnlyUntil.equals(s.readOnlyUntil); - } -} diff --git a/javatests/com/google/gerrit/server/notedb/NoteDbChangeStateTest.java b/javatests/com/google/gerrit/server/notedb/NoteDbChangeStateTest.java deleted file mode 100644 index c82135e116..0000000000 --- a/javatests/com/google/gerrit/server/notedb/NoteDbChangeStateTest.java +++ /dev/null @@ -1,241 +0,0 @@ -// Copyright (C) 2016 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.base.Preconditions.checkArgument; -import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; -import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.NOTE_DB; -import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; -import static com.google.gerrit.server.notedb.NoteDbChangeState.applyDelta; -import static com.google.gerrit.server.notedb.NoteDbChangeState.parse; -import static com.google.gerrit.server.util.time.TimeUtil.nowTs; -import static org.eclipse.jgit.lib.ObjectId.zeroId; - -import com.google.common.collect.ImmutableMap; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.server.notedb.NoteDbChangeState.Delta; -import com.google.gerrit.server.notedb.NoteDbChangeState.RefState; -import com.google.gerrit.testing.GerritBaseTests; -import com.google.gerrit.testing.TestChanges; -import com.google.gerrit.testing.TestTimeUtil; -import java.sql.Timestamp; -import java.util.Optional; -import java.util.concurrent.TimeUnit; -import org.eclipse.jgit.lib.ObjectId; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; - -/** Unit tests for {@link NoteDbChangeState}. */ -public class NoteDbChangeStateTest extends GerritBaseTests { - ObjectId SHA1 = ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); - ObjectId SHA2 = ObjectId.fromString("abcd1234abcd1234abcd1234abcd1234abcd1234"); - ObjectId SHA3 = ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee"); - - @Before - public void setUp() { - TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS); - } - - @After - public void tearDown() { - TestTimeUtil.useSystemTime(); - } - - @Test - public void parseReviewDbWithoutDrafts() { - NoteDbChangeState state = parse(new Change.Id(1), SHA1.name()); - assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB); - assertThat(state.getChangeId()).isEqualTo(new Change.Id(1)); - assertThat(state.getChangeMetaId()).isEqualTo(SHA1); - assertThat(state.getDraftIds()).isEmpty(); - assertThat(state.getReadOnlyUntil()).isEmpty(); - assertThat(state.toString()).isEqualTo(SHA1.name()); - - state = parse(new Change.Id(1), "R," + SHA1.name()); - assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB); - assertThat(state.getChangeId()).isEqualTo(new Change.Id(1)); - assertThat(state.getChangeMetaId()).isEqualTo(SHA1); - assertThat(state.getDraftIds()).isEmpty(); - assertThat(state.getReadOnlyUntil()).isEmpty(); - assertThat(state.toString()).isEqualTo(SHA1.name()); - } - - @Test - public void parseReviewDbWithDrafts() { - String str = SHA1.name() + ",2003=" + SHA2.name() + ",1001=" + SHA3.name(); - String expected = SHA1.name() + ",1001=" + SHA3.name() + ",2003=" + SHA2.name(); - NoteDbChangeState state = parse(new Change.Id(1), str); - assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB); - assertThat(state.getChangeId()).isEqualTo(new Change.Id(1)); - assertThat(state.getChangeMetaId()).isEqualTo(SHA1); - assertThat(state.getDraftIds()) - .containsExactly( - new Account.Id(1001), SHA3, - new Account.Id(2003), SHA2); - assertThat(state.getReadOnlyUntil()).isEmpty(); - assertThat(state.toString()).isEqualTo(expected); - - state = parse(new Change.Id(1), "R," + str); - assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB); - assertThat(state.getChangeId()).isEqualTo(new Change.Id(1)); - assertThat(state.getChangeMetaId()).isEqualTo(SHA1); - assertThat(state.getDraftIds()) - .containsExactly( - new Account.Id(1001), SHA3, - new Account.Id(2003), SHA2); - assertThat(state.getReadOnlyUntil()).isEmpty(); - assertThat(state.toString()).isEqualTo(expected); - } - - @Test - public void parseReadOnlyUntil() { - Timestamp ts = new Timestamp(12345); - String str = "R=12345," + SHA1.name(); - NoteDbChangeState state = parse(new Change.Id(1), str); - assertThat(state.getPrimaryStorage()).isEqualTo(REVIEW_DB); - assertThat(state.getChangeId()).isEqualTo(new Change.Id(1)); - assertThat(state.getChangeMetaId()).isEqualTo(SHA1); - assertThat(state.getReadOnlyUntil().get()).isEqualTo(ts); - assertThat(state.toString()).isEqualTo(str); - - str = "N=12345"; - state = parse(new Change.Id(1), str); - assertThat(state.getPrimaryStorage()).isEqualTo(NOTE_DB); - assertThat(state.getChangeId()).isEqualTo(new Change.Id(1)); - assertThat(state.getRefState()).isEmpty(); - assertThat(state.getReadOnlyUntil().get()).isEqualTo(ts); - assertThat(state.toString()).isEqualTo(str); - } - - @Test - public void applyDeltaToNullWithNoNewMetaId() throws Exception { - Change c = newChange(); - assertThat(c.getNoteDbState()).isNull(); - applyDelta(c, Delta.create(c.getId(), noMetaId(), noDrafts())); - assertThat(c.getNoteDbState()).isNull(); - - applyDelta(c, Delta.create(c.getId(), noMetaId(), drafts(new Account.Id(1001), zeroId()))); - assertThat(c.getNoteDbState()).isNull(); - } - - @Test - public void applyDeltaToMetaId() throws Exception { - Change c = newChange(); - applyDelta(c, Delta.create(c.getId(), metaId(SHA1), noDrafts())); - assertThat(c.getNoteDbState()).isEqualTo(SHA1.name()); - - applyDelta(c, Delta.create(c.getId(), metaId(SHA2), noDrafts())); - assertThat(c.getNoteDbState()).isEqualTo(SHA2.name()); - - // No-op delta. - applyDelta(c, Delta.create(c.getId(), noMetaId(), noDrafts())); - assertThat(c.getNoteDbState()).isEqualTo(SHA2.name()); - - // Set to zero clears the field. - applyDelta(c, Delta.create(c.getId(), metaId(zeroId()), noDrafts())); - assertThat(c.getNoteDbState()).isNull(); - } - - @Test - public void applyDeltaToDrafts() throws Exception { - Change c = newChange(); - applyDelta(c, Delta.create(c.getId(), metaId(SHA1), drafts(new Account.Id(1001), SHA2))); - assertThat(c.getNoteDbState()).isEqualTo(SHA1.name() + ",1001=" + SHA2.name()); - - applyDelta(c, Delta.create(c.getId(), noMetaId(), drafts(new Account.Id(2003), SHA3))); - assertThat(c.getNoteDbState()) - .isEqualTo(SHA1.name() + ",1001=" + SHA2.name() + ",2003=" + SHA3.name()); - - applyDelta(c, Delta.create(c.getId(), noMetaId(), drafts(new Account.Id(2003), zeroId()))); - assertThat(c.getNoteDbState()).isEqualTo(SHA1.name() + ",1001=" + SHA2.name()); - - applyDelta(c, Delta.create(c.getId(), metaId(SHA3), noDrafts())); - assertThat(c.getNoteDbState()).isEqualTo(SHA3.name() + ",1001=" + SHA2.name()); - } - - @Test - public void applyDeltaToReadOnly() throws Exception { - Timestamp ts = nowTs(); - Change c = newChange(); - NoteDbChangeState state = - new NoteDbChangeState( - c.getId(), - REVIEW_DB, - Optional.of(RefState.create(SHA1, ImmutableMap.of())), - Optional.of(new Timestamp(ts.getTime() + 10000))); - c.setNoteDbState(state.toString()); - Delta delta = Delta.create(c.getId(), metaId(SHA2), noDrafts()); - applyDelta(c, delta); - assertThat(NoteDbChangeState.parse(c)) - .isEqualTo( - new NoteDbChangeState( - state.getChangeId(), - state.getPrimaryStorage(), - Optional.of(RefState.create(SHA2, ImmutableMap.of())), - state.getReadOnlyUntil())); - } - - @Test - public void parseNoteDbPrimary() { - NoteDbChangeState state = parse(new Change.Id(1), "N"); - assertThat(state.getPrimaryStorage()).isEqualTo(NOTE_DB); - assertThat(state.getRefState()).isEmpty(); - assertThat(state.getReadOnlyUntil()).isEmpty(); - } - - @Test(expected = IllegalArgumentException.class) - public void parseInvalidPrimaryStorage() { - parse(new Change.Id(1), "X"); - } - - @Test - public void applyDeltaToNoteDbPrimaryIsNoOp() throws Exception { - Change c = newChange(); - c.setNoteDbState("N"); - applyDelta(c, Delta.create(c.getId(), metaId(SHA1), drafts(new Account.Id(1001), SHA2))); - assertThat(c.getNoteDbState()).isEqualTo("N"); - } - - private static Change newChange() { - return TestChanges.newChange(new Project.NameKey("project"), new Account.Id(12345)); - } - - // Static factory methods to avoid type arguments when using as method args. - - private static Optional noMetaId() { - return Optional.empty(); - } - - private static Optional metaId(ObjectId id) { - return Optional.of(id); - } - - private static ImmutableMap noDrafts() { - return ImmutableMap.of(); - } - - private static ImmutableMap drafts(Object... args) { - checkArgument(args.length % 2 == 0); - ImmutableMap.Builder b = ImmutableMap.builder(); - for (int i = 0; i < args.length / 2; i++) { - b.put((Account.Id) args[2 * i], (ObjectId) args[2 * i + 1]); - } - return b.build(); - } -} diff --git a/proto/reviewdb.proto b/proto/reviewdb.proto index bc45eeabc9..a958b0c33b 100644 --- a/proto/reviewdb.proto +++ b/proto/reviewdb.proto @@ -51,14 +51,14 @@ message Change { optional bool work_in_progress = 21; optional bool review_started = 22; optional Change_Id revert_of = 23; - optional string note_db_state = 101; // Deleted fields, should not be reused: - reserved 6; // sortkey - reserved 9; // open - reserved 11; // nbrPatchSets - reserved 15; // lastSha1MergeTested - reserved 16; // mergeable + reserved 6; // sortkey + reserved 9; // open + reserved 11; // nbrPatchSets + reserved 15; // lastSha1MergeTested + reserved 16; // mergeable + reserved 101; // note_db_state } // Serialized form of com.google.gerrit.reviewdb.client.ChangeMessage.