Remove sequence-based UID generation for messages

Unique identifiers for ChangeMessages and PatchLineComments were
generated using a seed from a database sequence, mixed together with
IdGenerator. In the spirit of killing ReviewDb, use a
cryptographically secure pseudorandom number generator to generate
these IDs instead. Follow the same "xxxxxxxx_xxxxxxxx" format so IDs
look similar.

I suppose the goal of the sequence-based approach was to decrease the
chance of collision by including a piece of state in each ID that is
guaranteed unique across the server. However, the chance of collision
for any 8-byte identifier can never be less than 1/2^64 no matter what
implementation we choose.

By definition a cryptographically secure PRNG generates random data
that is evenly distributed across the entire byte space, so the PRNG
implementation also has a 1/2^64 chance of collisions; we're just
swapping one rare-collision implementation with another. (This assumes
no bugs or weaknesses in the PRNG implementation, but with all due
respect to Shawn, I don't think the JDK's PRNG is any more likely to
have collision-introducing flaws than our hand-written IdGenerator.)

This change actually has no effect on ChangeMessage IDs that are
stored in or read from NoteDb, because ChangeNotesParser always
populates the ChangeMessage UUID field with the SHA-1 in the notes
graph of the commit that introduced the message; ReviewDb UUIDs are
simply discarded. The messageUuid method is still called during
creation because of the implementation details of ChangeMessagesUtil,
but that ID is not used for storage.

This change does have an effect on the IDs generated for
PatchLineComments, which also use messageUuid for their IDs, despite
the somewhat confusing fact that the ReviewDb interface implies the ID
is just for ChangeMessages.

While we're in there, rename the method to messageUuid to match the
Google Java Style Guide for camelCase names.

Change-Id: I7d157b9e8f87b41d4e7a146b705012863c27d42e
This commit is contained in:
Dave Borowitz 2017-01-05 12:30:27 -05:00
parent 2428f4fce9
commit 5a142312e9
22 changed files with 75 additions and 237 deletions

View File

@ -457,7 +457,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
public boolean updateChange(ChangeContext ctx) throws OrmException {
PatchSet.Id psId = ctx.getChange().currentPatchSetId();
ChangeMessage cm = new ChangeMessage(
new ChangeMessage.Key(id, ChangeUtil.messageUUID(ctx.getDb())),
new ChangeMessage.Key(id, ChangeUtil.messageUuid()),
ctx.getAccountId(), ctx.getWhen(), psId);
cm.setMessage(msg);
ctx.getDb().changeMessages().insert(Collections.singleton(cm));
@ -1278,7 +1278,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
private ChangeMessage insertMessage(Change.Id id, PatchSet.Id psId,
Account.Id author, Timestamp ts, String message) throws Exception {
ChangeMessage msg = new ChangeMessage(
new ChangeMessage.Key(id, ChangeUtil.messageUUID(db)),
new ChangeMessage.Key(id, ChangeUtil.messageUuid()),
author, ts, psId);
msg.setMessage(message);
db.changeMessages().insert(Collections.singleton(msg));

View File

@ -17,7 +17,6 @@ package com.google.gerrit.reviewdb.server;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.SystemConfig;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.Relation;
@ -118,12 +117,4 @@ public interface ReviewDb extends Schema {
@Sequence(startWith = FIRST_CHANGE_ID)
@Deprecated
int nextChangeId() throws OrmException;
/**
* Next id for a block of {@link ChangeMessage} records.
*
* @see com.google.gerrit.server.ChangeUtil#messageUUID(ReviewDb)
*/
@Sequence
int nextChangeMessageId() throws OrmException;
}

View File

@ -159,11 +159,6 @@ public class ReviewDbWrapper implements ReviewDb {
return delegate.nextChangeId();
}
@Override
public int nextChangeMessageId() throws OrmException {
return delegate.nextChangeMessageId();
}
public static class ChangeAccessWrapper implements ChangeAccess {
protected final ChangeAccess delegate;

View File

@ -73,20 +73,20 @@ public class ChangeMessagesUtil {
public static final String TAG_UPLOADED_PATCH_SET =
"autogenerated:gerrit:newPatchSet";
public static ChangeMessage newMessage( BatchUpdate.ChangeContext ctx,
String body, @Nullable String tag) throws OrmException {
public static ChangeMessage newMessage(BatchUpdate.ChangeContext ctx,
String body, @Nullable String tag) {
return newMessage(
ctx.getDb(), ctx.getChange().currentPatchSetId(),
ctx.getChange().currentPatchSetId(),
ctx.getUser(), ctx.getWhen(), body, tag);
}
public static ChangeMessage newMessage(
ReviewDb db, PatchSet.Id psId, CurrentUser user, Timestamp when,
String body, @Nullable String tag) throws OrmException {
PatchSet.Id psId, CurrentUser user, Timestamp when,
String body, @Nullable String tag) {
checkNotNull(psId);
Account.Id accountId = user.isInternalUser() ? null : user.getAccountId();
ChangeMessage m = new ChangeMessage(
new ChangeMessage.Key(psId.getParentKey(), ChangeUtil.messageUUID(db)),
new ChangeMessage.Key(psId.getParentKey(), ChangeUtil.messageUuid()),
accountId, when, psId);
m.setMessage(body);
m.setTag(tag);

View File

@ -17,10 +17,8 @@ package com.google.gerrit.server;
import static java.util.Comparator.comparingInt;
import com.google.common.collect.Ordering;
import com.google.common.io.BaseEncoding;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.util.IdGenerator;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.Ref;
@ -28,14 +26,15 @@ import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
import java.io.IOException;
import java.security.SecureRandom;
import java.util.Map;
import java.util.Random;
@Singleton
public class ChangeUtil {
private static final Object uuidLock = new Object();
private static final int SEED = 0x2418e6f9;
private static int uuidPrefix;
private static int uuidSeq;
private static final Random UUID_RANDOM = new SecureRandom();
private static final BaseEncoding UUID_ENCODING =
BaseEncoding.base16().lowerCase();
private static final int SUBJECT_MAX_LENGTH = 80;
private static final String SUBJECT_CROP_APPENDIX = "...";
@ -44,28 +43,12 @@ public class ChangeUtil {
public static final Ordering<PatchSet> PS_ID_ORDER =
Ordering.from(comparingInt(PatchSet::getPatchSetId));
/**
* Generate a new unique identifier for change message entities.
*
* @param db the database connection, used to increment the change message
* allocation sequence.
* @return the new unique identifier.
* @throws OrmException the database couldn't be incremented.
*/
public static String messageUUID(ReviewDb db) throws OrmException {
int p;
int s;
synchronized (uuidLock) {
if (uuidSeq == 0) {
uuidPrefix = db.nextChangeMessageId();
uuidSeq = Integer.MAX_VALUE;
}
p = uuidPrefix;
s = uuidSeq--;
}
String u = IdGenerator.format(IdGenerator.mix(SEED, p));
String l = IdGenerator.format(IdGenerator.mix(p, s));
return u + '_' + l;
/** @return a new unique identifier for change message entities. */
public static String messageUuid() {
byte[] buf = new byte[8];
UUID_RANDOM.nextBytes(buf);
return UUID_ENCODING.encode(buf, 0, 4) + '_'
+ UUID_ENCODING.encode(buf, 4, 4);
}
public static PatchSet.Id nextPatchSetId(Map<String, Ref> allRefs,

View File

@ -166,7 +166,7 @@ public class CommentsUtil {
}
}
Comment c = new Comment(
new Comment.Key(ChangeUtil.messageUUID(ctx.getDb()), path, psId.get()),
new Comment.Key(ChangeUtil.messageUuid(), path, psId.get()),
ctx.getUser().getAccountId(), ctx.getWhen(), side, message, serverId,
unresolved);
ctx.getUser().updateRealAccountId(c::setRealAuthor);
@ -175,9 +175,9 @@ public class CommentsUtil {
public RobotComment newRobotComment(ChangeContext ctx, String path,
PatchSet.Id psId, short side, String message, String robotId,
String robotRunId) throws OrmException {
String robotRunId) {
RobotComment c = new RobotComment(
new Comment.Key(ChangeUtil.messageUUID(ctx.getDb()), path, psId.get()),
new Comment.Key(ChangeUtil.messageUuid(), path, psId.get()),
ctx.getUser().getAccountId(), ctx.getWhen(), side, message, serverId,
robotId, robotRunId);
ctx.getUser().updateRealAccountId(c::setRealAuthor);

View File

@ -381,7 +381,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
db, update, labelTypes, patchSet, ctx.getControl(), approvals);
if (message != null) {
changeMessage = ChangeMessagesUtil.newMessage(
db, patchSet.getId(), ctx.getUser(), patchSet.getCreatedOn(),
patchSet.getId(), ctx.getUser(), patchSet.getCreatedOn(),
message, ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET);
cmUtil.addChangeMessage(db, update, changeMessage);
}

View File

@ -290,7 +290,7 @@ public class CherryPickChange {
.append(" as commit ")
.append(cherryPickCommit.name());
ChangeMessage changeMessage = ChangeMessagesUtil.newMessage(
ctx.getDb(), psId, ctx.getUser(), ctx.getWhen(), sb.toString(),
psId, ctx.getUser(), ctx.getWhen(), sb.toString(),
ChangeMessagesUtil.TAG_CHERRY_PICK_CHANGE);
cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), changeMessage);
return true;

View File

@ -250,7 +250,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
if (message != null) {
changeMessage = ChangeMessagesUtil.newMessage(
db, patchSet.getId(), ctx.getUser(), ctx.getWhen(), message,
patchSet.getId(), ctx.getUser(), ctx.getWhen(), message,
ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET);
changeMessage.setMessage(message);
}

View File

@ -846,13 +846,13 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
robotCommentInput.range);
robotComment.tag = in.tag;
setCommentRevId(robotComment, patchListCache, ctx.getChange(), ps);
robotComment.fixSuggestions = createFixSuggestionsFromInput(ctx,
robotCommentInput.fixSuggestions);
robotComment.fixSuggestions =
createFixSuggestionsFromInput(robotCommentInput.fixSuggestions);
return robotComment;
}
private List<FixSuggestion> createFixSuggestionsFromInput(ChangeContext ctx,
List<FixSuggestionInfo> fixSuggestionInfos) throws OrmException {
private List<FixSuggestion> createFixSuggestionsFromInput(
List<FixSuggestionInfo> fixSuggestionInfos) {
if (fixSuggestionInfos == null) {
return Collections.emptyList();
}
@ -860,17 +860,16 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
List<FixSuggestion> fixSuggestions =
new ArrayList<>(fixSuggestionInfos.size());
for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) {
fixSuggestions.add(
createFixSuggestionFromInput(ctx, fixSuggestionInfo));
fixSuggestions.add(createFixSuggestionFromInput(fixSuggestionInfo));
}
return fixSuggestions;
}
private FixSuggestion createFixSuggestionFromInput(ChangeContext ctx,
FixSuggestionInfo fixSuggestionInfo) throws OrmException {
private FixSuggestion createFixSuggestionFromInput(
FixSuggestionInfo fixSuggestionInfo) {
List<FixReplacement> fixReplacements =
toFixReplacements(fixSuggestionInfo.replacements);
String fixId = ChangeUtil.messageUUID(ctx.getDb());
String fixId = ChangeUtil.messageUuid();
return new FixSuggestion(fixId, fixSuggestionInfo.description,
fixReplacements);
}
@ -1235,7 +1234,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
message = ChangeMessagesUtil.newMessage(
ctx.getDb(), psId, user, ctx.getWhen(),
psId, user, ctx.getWhen(),
"Patch Set " + psId.get() + ":" + buf, in.tag);
cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), message);
return true;

View File

@ -235,7 +235,7 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
} else {
sendReplacePatchSet(ctx);
}
} catch (EmailException | OrmException e) {
} catch (EmailException e) {
log.error("Cannot send email for publishing draft " + psId, e);
}
}
@ -250,10 +250,9 @@ public class PublishDraftPatchSet implements RestModifyView<RevisionResource, In
cm.send();
}
private void sendReplacePatchSet(Context ctx)
throws EmailException, OrmException {
private void sendReplacePatchSet(Context ctx) throws EmailException {
ChangeMessage msg = ChangeMessagesUtil.newMessage(
ctx.getDb(), psId, ctx.getUser(), ctx.getWhen(),
psId, ctx.getUser(), ctx.getWhen(),
"Uploaded patch set " + psId.get() + ".",
ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET);
ReplacePatchSetSender cm =

View File

@ -117,7 +117,7 @@ public class PutDescription implements RestModifyView<RevisionResource,
ctx.getDb().patchSets().update(Collections.singleton(ps));
ChangeMessage cmsg =
ChangeMessagesUtil.newMessage(ctx.getDb(), psId, ctx.getUser(),
ChangeMessagesUtil.newMessage(psId, ctx.getUser(),
ctx.getWhen(), summary, ChangeMessagesUtil.TAG_SET_DESCRIPTION);
cmUtil.addChangeMessage(ctx.getDb(), update, cmsg);
return true;

View File

@ -123,7 +123,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
return true;
}
private ChangeMessage newMessage(ChangeContext ctx) throws OrmException {
private ChangeMessage newMessage(ChangeContext ctx) {
StringBuilder msg = new StringBuilder();
msg.append("Restored");
if (!Strings.nullToEmpty(input.message).trim().isEmpty()) {

View File

@ -111,7 +111,7 @@ public class AbandonOp extends BatchUpdate.Op {
return true;
}
private ChangeMessage newMessage(ChangeContext ctx) throws OrmException {
private ChangeMessage newMessage(ChangeContext ctx) {
StringBuilder msg = new StringBuilder();
msg.append("Abandoned");
if (!Strings.nullToEmpty(msgTxt).trim().isEmpty()) {

View File

@ -761,7 +761,7 @@ public class MergeOp implements AutoCloseable {
change.setStatus(Change.Status.ABANDONED);
ChangeMessage msg = ChangeMessagesUtil.newMessage(
ctx.getDb(), change.currentPatchSetId(),
change.currentPatchSetId(),
internalUserFactory.create(), change.getLastUpdatedOn(),
ChangeMessagesUtil.TAG_MERGED,
"Project was deleted.");

View File

@ -151,7 +151,7 @@ public class MergedByPushOp extends BatchUpdate.Op {
}
msgBuf.append(".");
ChangeMessage msg = ChangeMessagesUtil.newMessage(
ctx.getDb(), psId, ctx.getUser(), ctx.getWhen(), msgBuf.toString(),
psId, ctx.getUser(), ctx.getWhen(), msgBuf.toString(),
ChangeMessagesUtil.TAG_MERGED);
cmUtil.addChangeMessage(ctx.getDb(), update, msg);

View File

@ -285,7 +285,7 @@ public class ReplaceOp extends BatchUpdate.Op {
if (!Strings.isNullOrEmpty(reviewMessage)) {
message.append("\n").append(reviewMessage);
}
msg = ChangeMessagesUtil.newMessage(ctx.getDb(), patchSetId, ctx.getUser(),
msg = ChangeMessagesUtil.newMessage(patchSetId, ctx.getUser(),
ctx.getWhen(), message.toString(), ChangeMessagesUtil.TAG_UPLOADED_PATCH_SET);
cmUtil.addChangeMessage(ctx.getDb(), update, msg);

View File

@ -449,9 +449,9 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
}
private ChangeMessage message(ChangeContext ctx, PatchSet.Id psId,
String body) throws OrmException {
String body) {
return ChangeMessagesUtil.newMessage(
ctx.getDb(), psId, ctx.getUser(), ctx.getWhen(), body,
psId, ctx.getUser(), ctx.getWhen(), body,
ChangeMessagesUtil.TAG_MERGED);
}

View File

@ -36,7 +36,7 @@ import java.util.concurrent.TimeUnit;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_139> C = Schema_139.class;
public static final Class<Schema_140> C = Schema_140.class;
public static int getBinaryVersion() {
return guessVersion(C);

View File

@ -0,0 +1,26 @@
// Copyright (C) 2017 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.schema;
import com.google.inject.Inject;
import com.google.inject.Provider;
/** Remove ChangeMessage sequence. */
public class Schema_140 extends SchemaVersion {
@Inject
Schema_140(Provider<Schema_139> prior) {
super(prior);
}
}

View File

@ -16,28 +16,8 @@ package com.google.gerrit.server;
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.reviewdb.server.AccountAccess;
import com.google.gerrit.reviewdb.server.AccountExternalIdAccess;
import com.google.gerrit.reviewdb.server.AccountGroupAccess;
import com.google.gerrit.reviewdb.server.AccountGroupByIdAccess;
import com.google.gerrit.reviewdb.server.AccountGroupByIdAudAccess;
import com.google.gerrit.reviewdb.server.AccountGroupMemberAccess;
import com.google.gerrit.reviewdb.server.AccountGroupMemberAuditAccess;
import com.google.gerrit.reviewdb.server.AccountGroupNameAccess;
import com.google.gerrit.reviewdb.server.ChangeAccess;
import com.google.gerrit.reviewdb.server.ChangeMessageAccess;
import com.google.gerrit.reviewdb.server.PatchLineCommentAccess;
import com.google.gerrit.reviewdb.server.PatchSetAccess;
import com.google.gerrit.reviewdb.server.PatchSetApprovalAccess;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.SchemaVersionAccess;
import com.google.gerrit.reviewdb.server.SystemConfigAccess;
import com.google.gwtorm.server.Access;
import com.google.gwtorm.server.StatementExecutor;
import org.junit.Test;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.regex.Pattern;
public class ChangeUtilTest {
@ -46,141 +26,11 @@ public class ChangeUtilTest {
Pattern pat = Pattern.compile("^[0-9a-f]{8}_[0-9a-f]{8}$");
assertThat("abcd1234_0987fedc").matches(pat);
ReviewDb db = new FakeDb();
String id1 = ChangeUtil.messageUUID(db);
String id1 = ChangeUtil.messageUuid();
assertThat(id1).matches(pat);
String id2 = ChangeUtil.messageUUID(db);
String id2 = ChangeUtil.messageUuid();
assertThat(id2).isNotEqualTo(id1);
assertThat(id2).matches(pat);
}
private static class FakeDb implements ReviewDb {
private final AtomicInteger SEQ = new AtomicInteger(100);
@Override
public int nextChangeMessageId() {
return SEQ.getAndIncrement();
}
@Override
public void commit() {
throw new UnsupportedOperationException();
}
@Override
public void rollback() {
throw new UnsupportedOperationException();
}
@Override
public void updateSchema(StatementExecutor e) {
throw new UnsupportedOperationException();
}
@Override
public void pruneSchema(StatementExecutor e) {
throw new UnsupportedOperationException();
}
@Override
public Access<?, ?>[] allRelations() {
throw new UnsupportedOperationException();
}
@Override
public void close() {
throw new UnsupportedOperationException();
}
@Override
public SchemaVersionAccess schemaVersion() {
throw new UnsupportedOperationException();
}
@Override
public SystemConfigAccess systemConfig() {
throw new UnsupportedOperationException();
}
@Override
public AccountAccess accounts() {
throw new UnsupportedOperationException();
}
@Override
public AccountExternalIdAccess accountExternalIds() {
throw new UnsupportedOperationException();
}
@Override
public AccountGroupAccess accountGroups() {
throw new UnsupportedOperationException();
}
@Override
public AccountGroupNameAccess accountGroupNames() {
throw new UnsupportedOperationException();
}
@Override
public AccountGroupMemberAccess accountGroupMembers() {
throw new UnsupportedOperationException();
}
@Override
public AccountGroupMemberAuditAccess accountGroupMembersAudit() {
throw new UnsupportedOperationException();
}
@Override
public ChangeAccess changes() {
throw new UnsupportedOperationException();
}
@Override
public PatchSetApprovalAccess patchSetApprovals() {
throw new UnsupportedOperationException();
}
@Override
public ChangeMessageAccess changeMessages() {
throw new UnsupportedOperationException();
}
@Override
public PatchSetAccess patchSets() {
throw new UnsupportedOperationException();
}
@Override
public PatchLineCommentAccess patchComments() {
throw new UnsupportedOperationException();
}
@Override
public AccountGroupByIdAccess accountGroupById() {
throw new UnsupportedOperationException();
}
@Override
public AccountGroupByIdAudAccess accountGroupByIdAud() {
throw new UnsupportedOperationException();
}
@Override
public int nextAccountId() {
throw new UnsupportedOperationException();
}
@Override
public int nextAccountGroupId() {
throw new UnsupportedOperationException();
}
@Override
public int nextChangeId() {
throw new UnsupportedOperationException();
}
}
}

View File

@ -162,9 +162,4 @@ public class DisabledReviewDb implements ReviewDb {
public int nextChangeId() {
throw new Disabled();
}
@Override
public int nextChangeMessageId() {
throw new Disabled();
}
}