From c0b6854e7b8b63d5eeffed220d24e0cdc308f29c Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 29 May 2018 17:42:03 +0200 Subject: [PATCH] Split ChangeNoteUtil into JSON/Read/Write parts The write part needs the AccountCache, which is problematic in the schema migration. Change-Id: I2b8a6a0d946c765588107fb10abd19b96aef274c --- .../server/notedb/AbstractChangeNotes.java | 9 +- .../server/notedb/AbstractChangeUpdate.java | 6 +- .../gerrit/server/notedb/ChangeBundle.java | 4 +- .../server/notedb/ChangeDraftUpdate.java | 14 +- .../gerrit/server/notedb/ChangeNoteJson.java | 49 ++ .../gerrit/server/notedb/ChangeNoteUtil.java | 617 +----------------- .../server/notedb/ChangeNotesCache.java | 8 +- .../server/notedb/ChangeNotesParser.java | 27 +- .../server/notedb/ChangeRevisionNote.java | 15 +- .../gerrit/server/notedb/ChangeUpdate.java | 19 +- .../server/notedb/DeleteCommentRewriter.java | 34 +- .../server/notedb/DraftCommentNotes.java | 3 +- .../server/notedb/LegacyChangeNoteRead.java | 402 ++++++++++++ .../server/notedb/LegacyChangeNoteWrite.java | 206 ++++++ .../gerrit/server/notedb/NoteDbUtil.java | 29 + .../server/notedb/RevisionNoteBuilder.java | 15 +- .../gerrit/server/notedb/RevisionNoteMap.java | 10 +- .../server/notedb/RobotCommentNotes.java | 3 +- .../server/notedb/RobotCommentUpdate.java | 3 +- .../notedb/RobotCommentsRevisionNote.java | 4 +- .../notedb/rebuild/ChangeRebuilderImpl.java | 7 +- .../acceptance/api/change/ChangeIT.java | 9 +- .../acceptance/git/RefAdvertisementIT.java | 4 +- .../rest/change/CreateChangeIT.java | 4 +- .../acceptance/server/change/CommentsIT.java | 2 +- .../server/change/ConsistencyCheckerIT.java | 4 +- .../server/change/LegacyCommentsIT.java | 2 +- .../server/notedb/NoteDbPrimaryIT.java | 2 +- .../server/notedb/ChangeNotesParserTest.java | 16 +- .../gerrit/server/notedb/ChangeNotesTest.java | 43 +- .../notedb/CommentTimestampAdapterTest.java | 2 +- 31 files changed, 906 insertions(+), 666 deletions(-) create mode 100644 java/com/google/gerrit/server/notedb/ChangeNoteJson.java create mode 100644 java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java create mode 100644 java/com/google/gerrit/server/notedb/LegacyChangeNoteWrite.java diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java index ef2c9b32db..a083a7168b 100644 --- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java @@ -48,7 +48,8 @@ public abstract class AbstractChangeNotes { final GitRepositoryManager repoManager; final NotesMigration migration; final AllUsersName allUsers; - final ChangeNoteUtil noteUtil; + final ChangeNoteJson changeNoteJson; + final LegacyChangeNoteRead legacyChangeNoteRead; final NoteDbMetrics metrics; final Provider db; @@ -65,7 +66,8 @@ public abstract class AbstractChangeNotes { GitRepositoryManager repoManager, NotesMigration migration, AllUsersName allUsers, - ChangeNoteUtil noteUtil, + ChangeNoteJson changeNoteJson, + LegacyChangeNoteRead legacyChangeNoteRead, NoteDbMetrics metrics, Provider db, Provider rebuilder, @@ -73,7 +75,8 @@ public abstract class AbstractChangeNotes { this.repoManager = repoManager; this.migration = migration; this.allUsers = allUsers; - this.noteUtil = noteUtil; + this.legacyChangeNoteRead = legacyChangeNoteRead; + this.changeNoteJson = changeNoteJson; this.metrics = metrics; this.db = db; this.rebuilder = rebuilder; diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index 010c5c0f8a..3653bc7d47 100644 --- a/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -120,7 +120,9 @@ public abstract class AbstractChangeUpdate { ChangeNoteUtil noteUtil, PersonIdent serverIdent, CurrentUser u, Date when) { checkUserType(u); if (u instanceof IdentifiedUser) { - return noteUtil.newIdent(u.asIdentifiedUser().getAccount(), when, serverIdent); + return noteUtil + .getLegacyChangeNoteWrite() + .newIdent(u.asIdentifiedUser().getAccount(), when, serverIdent); } else if (u instanceof InternalUser) { return serverIdent; } @@ -175,7 +177,7 @@ public abstract class AbstractChangeUpdate { } protected PersonIdent newIdent(Account.Id authorId, Date when) { - return noteUtil.newIdent(authorId, when, serverIdent); + return noteUtil.getLegacyChangeNoteWrite().newIdent(authorId, when, serverIdent); } /** Whether no updates have been done. */ diff --git a/java/com/google/gerrit/server/notedb/ChangeBundle.java b/java/com/google/gerrit/server/notedb/ChangeBundle.java index 7714c6ee2b..1d3c7522ff 100644 --- a/java/com/google/gerrit/server/notedb/ChangeBundle.java +++ b/java/com/google/gerrit/server/notedb/ChangeBundle.java @@ -505,11 +505,11 @@ public class ChangeBundle { if (rn >= 0) { s = s.substring(0, rn); } - return ChangeNoteUtil.sanitizeFooter(s); + return NoteDbUtil.sanitizeFooter(s); } private static String cleanNoteDbSubject(String s) { - return ChangeNoteUtil.sanitizeFooter(s); + return NoteDbUtil.sanitizeFooter(s); } /** diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index 71c0b9e7ad..6b4bea7d22 100644 --- a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -178,7 +178,12 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { for (Map.Entry e : builders.entrySet()) { updatedRevs.add(e.getKey()); ObjectId id = ObjectId.fromString(e.getKey().get()); - byte[] data = e.getValue().build(noteUtil, noteUtil.getWriteJson()); + byte[] data = + e.getValue() + .build( + noteUtil.getChangeNoteJson(), + noteUtil.getLegacyChangeNoteWrite(), + noteUtil.getChangeNoteJson().getWriteJson()); if (!Arrays.equals(data, e.getValue().baseRaw)) { touchedAnyRevs = true; } @@ -236,7 +241,12 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { // Even though reading from changes might not be enabled, we need to // parse any existing revision notes so we can merge them. return RevisionNoteMap.parse( - noteUtil, getId(), rw.getObjectReader(), noteMap, PatchLineComment.Status.DRAFT); + noteUtil.getChangeNoteJson(), + noteUtil.getLegacyChangeNoteRead(), + getId(), + rw.getObjectReader(), + noteMap, + PatchLineComment.Status.DRAFT); } @Override diff --git a/java/com/google/gerrit/server/notedb/ChangeNoteJson.java b/java/com/google/gerrit/server/notedb/ChangeNoteJson.java new file mode 100644 index 0000000000..0475fe3fcb --- /dev/null +++ b/java/com/google/gerrit/server/notedb/ChangeNoteJson.java @@ -0,0 +1,49 @@ +// Copyright (C) 2018 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 com.google.gerrit.server.config.GerritServerConfig; +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.sql.Timestamp; +import org.eclipse.jgit.lib.Config; + +@Singleton +public class ChangeNoteJson { + private final Gson gson = newGson(); + private final boolean writeJson; + + static Gson newGson() { + return new GsonBuilder() + .registerTypeAdapter(Timestamp.class, new CommentTimestampAdapter().nullSafe()) + .setPrettyPrinting() + .create(); + } + + public Gson getGson() { + return gson; + } + + public boolean getWriteJson() { + return writeJson; + } + + @Inject + ChangeNoteJson(@GerritServerConfig Config config) { + this.writeJson = config.getBoolean("notedb", "writeJson", true); + } +} diff --git a/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java index e8e4decf0a..070f974df0 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java +++ b/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java @@ -14,52 +14,8 @@ package com.google.gerrit.server.notedb; -import static com.google.common.base.Preconditions.checkArgument; -import static com.google.gerrit.server.CommentsUtil.COMMENT_ORDER; -import static com.google.gerrit.server.notedb.ChangeNotes.parseException; -import static java.nio.charset.StandardCharsets.UTF_8; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.CharMatcher; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ListMultimap; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.Comment; -import com.google.gerrit.reviewdb.client.CommentRange; -import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.RevId; -import com.google.gerrit.server.GerritPersonIdent; -import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.account.AccountState; -import com.google.gerrit.server.config.GerritServerConfig; -import com.google.gerrit.server.config.GerritServerId; -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; import com.google.inject.Inject; -import java.io.OutputStream; -import java.io.OutputStreamWriter; -import java.io.PrintWriter; -import java.sql.Timestamp; -import java.text.ParseException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Date; -import java.util.HashSet; -import java.util.List; -import java.util.Locale; -import java.util.Optional; -import java.util.Set; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.revwalk.FooterKey; -import org.eclipse.jgit.util.GitDateFormatter; -import org.eclipse.jgit.util.GitDateFormatter.Format; -import org.eclipse.jgit.util.GitDateParser; -import org.eclipse.jgit.util.MutableInteger; -import org.eclipse.jgit.util.QuotedString; -import org.eclipse.jgit.util.RawParseUtils; public class ChangeNoteUtil { public static final FooterKey FOOTER_ASSIGNEE = new FooterKey("Assignee"); @@ -85,560 +41,43 @@ public class ChangeNoteUtil { public static final FooterKey FOOTER_WORK_IN_PROGRESS = new FooterKey("Work-in-progress"); public static final FooterKey FOOTER_REVERT_OF = new FooterKey("Revert-of"); - private static final String AUTHOR = "Author"; - private static final String BASE_PATCH_SET = "Base-for-patch-set"; - private static final String COMMENT_RANGE = "Comment-range"; - private static final String FILE = "File"; - private static final String LENGTH = "Bytes"; - private static final String PARENT = "Parent"; - private static final String PARENT_NUMBER = "Parent-number"; - private static final String PATCH_SET = "Patch-set"; - private static final String REAL_AUTHOR = "Real-author"; - private static final String REVISION = "Revision"; - private static final String UUID = "UUID"; - private static final String UNRESOLVED = "Unresolved"; - private static final String TAG = FOOTER_TAG.getName(); + static final String AUTHOR = "Author"; + static final String BASE_PATCH_SET = "Base-for-patch-set"; + static final String COMMENT_RANGE = "Comment-range"; + static final String FILE = "File"; + static final String LENGTH = "Bytes"; + static final String PARENT = "Parent"; + static final String PARENT_NUMBER = "Parent-number"; + static final String PATCH_SET = "Patch-set"; + static final String REAL_AUTHOR = "Real-author"; + static final String REVISION = "Revision"; + static final String UUID = "UUID"; + static final String UNRESOLVED = "Unresolved"; + static final String TAG = FOOTER_TAG.getName(); - public static String formatTime(PersonIdent ident, Timestamp t) { - GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT); - // TODO(dborowitz): Use a ThreadLocal or use Joda. - PersonIdent newIdent = new PersonIdent(ident, t); - return dateFormatter.formatDate(newIdent); - } - - static Gson newGson() { - return new GsonBuilder() - .registerTypeAdapter(Timestamp.class, new CommentTimestampAdapter().nullSafe()) - .setPrettyPrinting() - .create(); - } - - private final AccountCache accountCache; - private final PersonIdent serverIdent; - private final String serverId; - private final Gson gson = newGson(); - private final boolean writeJson; + private final LegacyChangeNoteRead legacyChangeNoteRead; + private final LegacyChangeNoteWrite legacyChangeNoteWrite; + private final ChangeNoteJson changeNoteJson; @Inject public ChangeNoteUtil( - AccountCache accountCache, - @GerritPersonIdent PersonIdent serverIdent, - @GerritServerId String serverId, - @GerritServerConfig Config config) { - this.accountCache = accountCache; - this.serverIdent = serverIdent; - this.serverId = serverId; - this.writeJson = config.getBoolean("notedb", "writeJson", true); + ChangeNoteJson changeNoteJson, + LegacyChangeNoteRead legacyChangeNoteRead, + LegacyChangeNoteWrite legacyChangeNoteWrite) { + this.changeNoteJson = changeNoteJson; + this.legacyChangeNoteRead = legacyChangeNoteRead; + this.legacyChangeNoteWrite = legacyChangeNoteWrite; } - public PersonIdent newIdent(Account.Id authorId, Date when, PersonIdent serverIdent) { - Optional author = accountCache.get(authorId).map(AccountState::getAccount); - return new PersonIdent( - author.map(Account::getName).orElseGet(() -> Account.getName(authorId)), - authorId.get() + "@" + serverId, - when, - serverIdent.getTimeZone()); + public LegacyChangeNoteRead getLegacyChangeNoteRead() { + return legacyChangeNoteRead; } - @VisibleForTesting - public PersonIdent newIdent(Account author, Date when, PersonIdent serverIdent) { - return new PersonIdent( - author.getName(), author.getId().get() + "@" + serverId, when, serverIdent.getTimeZone()); + public ChangeNoteJson getChangeNoteJson() { + return changeNoteJson; } - public boolean getWriteJson() { - return writeJson; - } - - public Gson getGson() { - return gson; - } - - public String getServerId() { - return serverId; - } - - public Account.Id parseIdent(PersonIdent ident, Change.Id changeId) - throws ConfigInvalidException { - return NoteDbUtil.parseIdent(ident, serverId) - .orElseThrow( - () -> - parseException( - changeId, - "invalid identity, expected @%s: %s", - serverId, - ident.getEmailAddress())); - } - - private static boolean match(byte[] note, MutableInteger p, byte[] expected) { - int m = RawParseUtils.match(note, p.value, expected); - return m == p.value + expected.length; - } - - public List parseNote(byte[] note, MutableInteger p, Change.Id changeId) - throws ConfigInvalidException { - if (p.value >= note.length) { - return ImmutableList.of(); - } - Set seen = new HashSet<>(); - List result = new ArrayList<>(); - int sizeOfNote = note.length; - byte[] psb = PATCH_SET.getBytes(UTF_8); - byte[] bpsb = BASE_PATCH_SET.getBytes(UTF_8); - byte[] bpn = PARENT_NUMBER.getBytes(UTF_8); - - RevId revId = new RevId(parseStringField(note, p, changeId, REVISION)); - String fileName = null; - PatchSet.Id psId = null; - boolean isForBase = false; - Integer parentNumber = null; - - while (p.value < sizeOfNote) { - boolean matchPs = match(note, p, psb); - boolean matchBase = match(note, p, bpsb); - if (matchPs) { - fileName = null; - psId = parsePsId(note, p, changeId, PATCH_SET); - isForBase = false; - } else if (matchBase) { - fileName = null; - psId = parsePsId(note, p, changeId, BASE_PATCH_SET); - isForBase = true; - if (match(note, p, bpn)) { - parentNumber = parseParentNumber(note, p, changeId); - } - } else if (psId == null) { - throw parseException(changeId, "missing %s or %s header", PATCH_SET, BASE_PATCH_SET); - } - - Comment c = parseComment(note, p, fileName, psId, revId, isForBase, parentNumber); - fileName = c.key.filename; - if (!seen.add(c.key)) { - throw parseException(changeId, "multiple comments for %s in note", c.key); - } - result.add(c); - } - return result; - } - - private Comment parseComment( - byte[] note, - MutableInteger curr, - String currentFileName, - PatchSet.Id psId, - RevId revId, - boolean isForBase, - Integer parentNumber) - throws ConfigInvalidException { - Change.Id changeId = psId.getParentKey(); - - // Check if there is a new file. - boolean newFile = (RawParseUtils.match(note, curr.value, FILE.getBytes(UTF_8))) != -1; - if (newFile) { - // If so, parse the new file name. - currentFileName = parseFilename(note, curr, changeId); - } else if (currentFileName == null) { - throw parseException(changeId, "could not parse %s", FILE); - } - - CommentRange range = parseCommentRange(note, curr); - if (range == null) { - throw parseException(changeId, "could not parse %s", COMMENT_RANGE); - } - - Timestamp commentTime = parseTimestamp(note, curr, changeId); - Account.Id aId = parseAuthor(note, curr, changeId, AUTHOR); - boolean hasRealAuthor = - (RawParseUtils.match(note, curr.value, REAL_AUTHOR.getBytes(UTF_8))) != -1; - Account.Id raId = null; - if (hasRealAuthor) { - raId = parseAuthor(note, curr, changeId, REAL_AUTHOR); - } - - boolean hasParent = (RawParseUtils.match(note, curr.value, PARENT.getBytes(UTF_8))) != -1; - String parentUUID = null; - boolean unresolved = false; - if (hasParent) { - parentUUID = parseStringField(note, curr, changeId, PARENT); - } - boolean hasUnresolved = - (RawParseUtils.match(note, curr.value, UNRESOLVED.getBytes(UTF_8))) != -1; - if (hasUnresolved) { - unresolved = parseBooleanField(note, curr, changeId, UNRESOLVED); - } - - String uuid = parseStringField(note, curr, changeId, UUID); - - boolean hasTag = (RawParseUtils.match(note, curr.value, TAG.getBytes(UTF_8))) != -1; - String tag = null; - if (hasTag) { - tag = parseStringField(note, curr, changeId, TAG); - } - - int commentLength = parseCommentLength(note, curr, changeId); - - String message = RawParseUtils.decode(UTF_8, note, curr.value, curr.value + commentLength); - checkResult(message, "message contents", changeId); - - Comment c = - new Comment( - new Comment.Key(uuid, currentFileName, psId.get()), - aId, - commentTime, - isForBase ? (short) (parentNumber == null ? 0 : -parentNumber) : (short) 1, - message, - serverId, - unresolved); - c.lineNbr = range.getEndLine(); - c.parentUuid = parentUUID; - c.tag = tag; - c.setRevId(revId); - if (raId != null) { - c.setRealAuthor(raId); - } - - if (range.getStartCharacter() != -1) { - c.setRange(range); - } - - curr.value = RawParseUtils.nextLF(note, curr.value + commentLength); - curr.value = RawParseUtils.nextLF(note, curr.value); - return c; - } - - private static String parseStringField( - byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) - throws ConfigInvalidException { - int endOfLine = RawParseUtils.nextLF(note, curr.value); - checkHeaderLineFormat(note, curr, fieldName, changeId); - int startOfField = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; - curr.value = endOfLine; - return RawParseUtils.decode(UTF_8, note, startOfField, endOfLine - 1); - } - - /** - * @return a comment range. If the comment range line in the note only has one number, we return a - * CommentRange with that one number as the end line and the other fields as -1. If the - * comment range line in the note contains a whole comment range, then we return a - * CommentRange with all fields set. If the line is not correctly formatted, return null. - */ - private static CommentRange parseCommentRange(byte[] note, MutableInteger ptr) { - CommentRange range = new CommentRange(-1, -1, -1, -1); - - int last = ptr.value; - int startLine = RawParseUtils.parseBase10(note, ptr.value, ptr); - if (ptr.value == last) { - return null; - } else if (note[ptr.value] == '\n') { - range.setEndLine(startLine); - ptr.value += 1; - return range; - } else if (note[ptr.value] == ':') { - range.setStartLine(startLine); - ptr.value += 1; - } else { - return null; - } - - last = ptr.value; - int startChar = RawParseUtils.parseBase10(note, ptr.value, ptr); - if (ptr.value == last) { - return null; - } else if (note[ptr.value] == '-') { - range.setStartCharacter(startChar); - ptr.value += 1; - } else { - return null; - } - - last = ptr.value; - int endLine = RawParseUtils.parseBase10(note, ptr.value, ptr); - if (ptr.value == last) { - return null; - } else if (note[ptr.value] == ':') { - range.setEndLine(endLine); - ptr.value += 1; - } else { - return null; - } - - last = ptr.value; - int endChar = RawParseUtils.parseBase10(note, ptr.value, ptr); - if (ptr.value == last) { - return null; - } else if (note[ptr.value] == '\n') { - range.setEndCharacter(endChar); - ptr.value += 1; - } else { - return null; - } - return range; - } - - private static PatchSet.Id parsePsId( - byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) - throws ConfigInvalidException { - checkHeaderLineFormat(note, curr, fieldName, changeId); - int startOfPsId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; - MutableInteger i = new MutableInteger(); - int patchSetId = RawParseUtils.parseBase10(note, startOfPsId, i); - int endOfLine = RawParseUtils.nextLF(note, curr.value); - if (i.value != endOfLine - 1) { - throw parseException(changeId, "could not parse %s", fieldName); - } - checkResult(patchSetId, "patchset id", changeId); - curr.value = endOfLine; - return new PatchSet.Id(changeId, patchSetId); - } - - private static Integer parseParentNumber(byte[] note, MutableInteger curr, Change.Id changeId) - throws ConfigInvalidException { - checkHeaderLineFormat(note, curr, PARENT_NUMBER, changeId); - - int start = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; - MutableInteger i = new MutableInteger(); - int parentNumber = RawParseUtils.parseBase10(note, start, i); - int endOfLine = RawParseUtils.nextLF(note, curr.value); - if (i.value != endOfLine - 1) { - throw parseException(changeId, "could not parse %s", PARENT_NUMBER); - } - checkResult(parentNumber, "parent number", changeId); - curr.value = endOfLine; - return Integer.valueOf(parentNumber); - } - - private static String parseFilename(byte[] note, MutableInteger curr, Change.Id changeId) - throws ConfigInvalidException { - checkHeaderLineFormat(note, curr, FILE, changeId); - int startOfFileName = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; - int endOfLine = RawParseUtils.nextLF(note, curr.value); - curr.value = endOfLine; - curr.value = RawParseUtils.nextLF(note, curr.value); - return QuotedString.GIT_PATH.dequote( - RawParseUtils.decode(UTF_8, note, startOfFileName, endOfLine - 1)); - } - - private static Timestamp parseTimestamp(byte[] note, MutableInteger curr, Change.Id changeId) - throws ConfigInvalidException { - int endOfLine = RawParseUtils.nextLF(note, curr.value); - Timestamp commentTime; - String dateString = RawParseUtils.decode(UTF_8, note, curr.value, endOfLine - 1); - try { - commentTime = new Timestamp(GitDateParser.parse(dateString, null, Locale.US).getTime()); - } catch (ParseException e) { - throw new ConfigInvalidException("could not parse comment timestamp", e); - } - curr.value = endOfLine; - return checkResult(commentTime, "comment timestamp", changeId); - } - - private Account.Id parseAuthor( - byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) - throws ConfigInvalidException { - checkHeaderLineFormat(note, curr, fieldName, changeId); - int startOfAccountId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; - PersonIdent ident = RawParseUtils.parsePersonIdent(note, startOfAccountId); - Account.Id aId = parseIdent(ident, changeId); - curr.value = RawParseUtils.nextLF(note, curr.value); - return checkResult(aId, fieldName, changeId); - } - - private static int parseCommentLength(byte[] note, MutableInteger curr, Change.Id changeId) - throws ConfigInvalidException { - checkHeaderLineFormat(note, curr, LENGTH, changeId); - int startOfLength = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; - MutableInteger i = new MutableInteger(); - i.value = startOfLength; - int commentLength = RawParseUtils.parseBase10(note, startOfLength, i); - if (i.value == startOfLength) { - throw parseException(changeId, "could not parse %s", LENGTH); - } - int endOfLine = RawParseUtils.nextLF(note, curr.value); - if (i.value != endOfLine - 1) { - throw parseException(changeId, "could not parse %s", LENGTH); - } - curr.value = endOfLine; - return checkResult(commentLength, "comment length", changeId); - } - - private boolean parseBooleanField( - byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) - throws ConfigInvalidException { - String str = parseStringField(note, curr, changeId, fieldName); - if ("true".equalsIgnoreCase(str)) { - return true; - } else if ("false".equalsIgnoreCase(str)) { - return false; - } - throw parseException(changeId, "invalid boolean for %s: %s", fieldName, str); - } - - private static T checkResult(T o, String fieldName, Change.Id changeId) - throws ConfigInvalidException { - if (o == null) { - throw parseException(changeId, "could not parse %s", fieldName); - } - return o; - } - - private static int checkResult(int i, String fieldName, Change.Id changeId) - throws ConfigInvalidException { - if (i <= 0) { - throw parseException(changeId, "could not parse %s", fieldName); - } - return i; - } - - private void appendHeaderField(PrintWriter writer, String field, String value) { - writer.print(field); - writer.print(": "); - writer.print(value); - writer.print('\n'); - } - - private static void checkHeaderLineFormat( - byte[] note, MutableInteger curr, String fieldName, Change.Id changeId) - throws ConfigInvalidException { - boolean correct = RawParseUtils.match(note, curr.value, fieldName.getBytes(UTF_8)) != -1; - int p = curr.value + fieldName.length(); - correct &= (p < note.length && note[p] == ':'); - p++; - correct &= (p < note.length && note[p] == ' '); - if (!correct) { - throw parseException(changeId, "could not parse %s", fieldName); - } - } - - /** - * Build a note that contains the metadata for and the contents of all of the comments in the - * given comments. - * - * @param comments Comments to be written to the output stream, keyed by patch set ID; multiple - * patch sets are allowed since base revisions may be shared across patch sets. All of the - * comments must share the same RevId, and all the comments for a given patch set must have - * the same side. - * @param out output stream to write to. - */ - void buildNote(ListMultimap comments, OutputStream out) { - if (comments.isEmpty()) { - return; - } - - List psIds = new ArrayList<>(comments.keySet()); - Collections.sort(psIds); - - OutputStreamWriter streamWriter = new OutputStreamWriter(out, UTF_8); - try (PrintWriter writer = new PrintWriter(streamWriter)) { - String revId = comments.values().iterator().next().revId; - appendHeaderField(writer, REVISION, revId); - - for (int psId : psIds) { - List psComments = COMMENT_ORDER.sortedCopy(comments.get(psId)); - Comment first = psComments.get(0); - - short side = first.side; - appendHeaderField(writer, side <= 0 ? BASE_PATCH_SET : PATCH_SET, Integer.toString(psId)); - if (side < 0) { - appendHeaderField(writer, PARENT_NUMBER, Integer.toString(-side)); - } - - String currentFilename = null; - - for (Comment c : psComments) { - checkArgument( - revId.equals(c.revId), - "All comments being added must have all the same RevId. The " - + "comment below does not have the same RevId as the others " - + "(%s).\n%s", - revId, - c); - checkArgument( - side == c.side, - "All comments being added must all have the same side. The " - + "comment below does not have the same side as the others " - + "(%s).\n%s", - side, - c); - String commentFilename = QuotedString.GIT_PATH.quote(c.key.filename); - - if (!commentFilename.equals(currentFilename)) { - currentFilename = commentFilename; - writer.print("File: "); - writer.print(commentFilename); - writer.print("\n\n"); - } - - appendOneComment(writer, c); - } - } - } - } - - private void appendOneComment(PrintWriter writer, Comment c) { - // The CommentRange field for a comment is allowed to be null. If it is - // null, then in the first line, we simply use the line number field for a - // comment instead. If it isn't null, we write the comment range itself. - Comment.Range range = c.range; - if (range != null) { - writer.print(range.startLine); - writer.print(':'); - writer.print(range.startChar); - writer.print('-'); - writer.print(range.endLine); - writer.print(':'); - writer.print(range.endChar); - } else { - writer.print(c.lineNbr); - } - writer.print("\n"); - - writer.print(formatTime(serverIdent, c.writtenOn)); - writer.print("\n"); - - appendIdent(writer, AUTHOR, c.author.getId(), c.writtenOn); - if (!c.getRealAuthor().equals(c.author)) { - appendIdent(writer, REAL_AUTHOR, c.getRealAuthor().getId(), c.writtenOn); - } - - String parent = c.parentUuid; - if (parent != null) { - appendHeaderField(writer, PARENT, parent); - } - - appendHeaderField(writer, UNRESOLVED, Boolean.toString(c.unresolved)); - appendHeaderField(writer, UUID, c.key.uuid); - - if (c.tag != null) { - appendHeaderField(writer, TAG, c.tag); - } - - byte[] messageBytes = c.message.getBytes(UTF_8); - appendHeaderField(writer, LENGTH, Integer.toString(messageBytes.length)); - - writer.print(c.message); - writer.print("\n\n"); - } - - private void appendIdent(PrintWriter writer, String header, Account.Id id, Timestamp ts) { - PersonIdent ident = newIdent(id, ts, serverIdent); - StringBuilder name = new StringBuilder(); - PersonIdent.appendSanitized(name, ident.getName()); - name.append(" <"); - PersonIdent.appendSanitized(name, ident.getEmailAddress()); - name.append('>'); - appendHeaderField(writer, header, name.toString()); - } - - private static final CharMatcher INVALID_FOOTER_CHARS = CharMatcher.anyOf("\r\n\0"); - - static String sanitizeFooter(String value) { - // Remove characters that would confuse JGit's footer parser if they were - // included in footer values, for example by splitting the footer block into - // multiple paragraphs. - // - // One painful example: RevCommit#getShorMessage() might return a message - // containing "\r\r", which RevCommit#getFooterLines() will treat as an - // empty paragraph for the purposes of footer parsing. - return INVALID_FOOTER_CHARS.trimAndCollapseFrom(value, ' '); + public LegacyChangeNoteWrite getLegacyChangeNoteWrite() { + return legacyChangeNoteWrite; } } diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java index 06d940eac5..0bf2108933 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java @@ -346,7 +346,13 @@ public class ChangeNotesCache { @Override public ChangeNotesState call() throws ConfigInvalidException, IOException { ChangeNotesParser parser = - new ChangeNotesParser(key.changeId(), key.id(), rw, args.noteUtil, args.metrics); + new ChangeNotesParser( + key.changeId(), + key.id(), + rw, + args.changeNoteJson, + args.legacyChangeNoteRead, + args.metrics); ChangeNotesState result = parser.parseAll(); // This assignment only happens if call() was actually called, which only // happens when Cache#get(K, Callable) incurs a cache miss. diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 2eb30ff20c..689b024d90 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -124,7 +124,9 @@ class ChangeNotesParser { } // Private final members initialized in the constructor. - private final ChangeNoteUtil noteUtil; + private final ChangeNoteJson changeNoteJson; + private final LegacyChangeNoteRead legacyChangeNoteRead; + private final NoteDbMetrics metrics; private final Change.Id id; private final ObjectId tip; @@ -175,12 +177,14 @@ class ChangeNotesParser { Change.Id changeId, ObjectId tip, ChangeNotesRevWalk walk, - ChangeNoteUtil noteUtil, + ChangeNoteJson changeNoteJson, + LegacyChangeNoteRead legacyChangeNoteRead, NoteDbMetrics metrics) { this.id = changeId; this.tip = tip; this.walk = walk; - this.noteUtil = noteUtil; + this.changeNoteJson = changeNoteJson; + this.legacyChangeNoteRead = legacyChangeNoteRead; this.metrics = metrics; approvals = new LinkedHashMap<>(); bufferedApprovals = new ArrayList<>(); @@ -446,7 +450,7 @@ class ChangeNotesParser { return effectiveAccountId; } PersonIdent ident = RawParseUtils.parsePersonIdent(realUser); - return noteUtil.parseIdent(ident, id); + return legacyChangeNoteRead.parseIdent(ident, id); } private String parseTopic(ChangeNotesCommit commit) throws ConfigInvalidException { @@ -581,7 +585,7 @@ class ChangeNotesParser { parsedAssignee = Optional.empty(); } else { PersonIdent ident = RawParseUtils.parsePersonIdent(assigneeValue); - parsedAssignee = Optional.ofNullable(noteUtil.parseIdent(ident, id)); + parsedAssignee = Optional.ofNullable(legacyChangeNoteRead.parseIdent(ident, id)); } if (assignee == null) { assignee = parsedAssignee; @@ -749,7 +753,8 @@ class ChangeNotesParser { ChangeNotesCommit tipCommit = walk.parseCommit(tip); revisionNoteMap = RevisionNoteMap.parse( - noteUtil, + changeNoteJson, + legacyChangeNoteRead, id, reader, NoteMap.read(reader, tipCommit), @@ -807,7 +812,7 @@ class ChangeNotesParser { labelVoteStr = line.substring(0, s); PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); checkFooter(ident != null, FOOTER_LABEL, line); - effectiveAccountId = noteUtil.parseIdent(ident, id); + effectiveAccountId = legacyChangeNoteRead.parseIdent(ident, id); } else { labelVoteStr = line; effectiveAccountId = committerId; @@ -849,7 +854,7 @@ class ChangeNotesParser { label = line.substring(1, s); PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); checkFooter(ident != null, FOOTER_LABEL, line); - effectiveAccountId = noteUtil.parseIdent(ident, id); + effectiveAccountId = legacyChangeNoteRead.parseIdent(ident, id); } else { label = line.substring(1); effectiveAccountId = committerId; @@ -913,7 +918,7 @@ class ChangeNotesParser { label.label = line.substring(c + 2, c2); PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(c2 + 2)); checkFooter(ident != null, FOOTER_SUBMITTED_WITH, line); - label.appliedBy = noteUtil.parseIdent(ident, id); + label.appliedBy = legacyChangeNoteRead.parseIdent(ident, id); } else { label.label = line.substring(c + 2); } @@ -929,7 +934,7 @@ class ChangeNotesParser { if (a.getName().equals(c.getName()) && a.getEmailAddress().equals(c.getEmailAddress())) { return null; } - return noteUtil.parseIdent(commit.getAuthorIdent(), id); + return legacyChangeNoteRead.parseIdent(commit.getAuthorIdent(), id); } private void parseReviewer(Timestamp ts, ReviewerStateInternal state, String line) @@ -938,7 +943,7 @@ class ChangeNotesParser { if (ident == null) { throw invalidFooter(state.getFooterKey(), line); } - Account.Id accountId = noteUtil.parseIdent(ident, id); + Account.Id accountId = legacyChangeNoteRead.parseIdent(ident, id); reviewerUpdates.add(ReviewerStatusUpdate.create(ts, ownerId, accountId, state)); if (!reviewers.containsRow(accountId)) { reviewers.put(accountId, state, ts); diff --git a/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java b/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java index 35e4a12812..894e9795e4 100644 --- a/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java +++ b/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java @@ -37,19 +37,22 @@ class ChangeRevisionNote extends RevisionNote { // See org.eclipse.jgit.transport.PushCertificateParser.END_SIGNATURE private static final byte[] END_SIGNATURE = "-----END PGP SIGNATURE-----\n".getBytes(UTF_8); - private final ChangeNoteUtil noteUtil; + private final ChangeNoteJson noteJson; + private final LegacyChangeNoteRead legacyChangeNoteRead; private final Change.Id changeId; private final PatchLineComment.Status status; private String pushCert; ChangeRevisionNote( - ChangeNoteUtil noteUtil, + ChangeNoteJson noteJson, + LegacyChangeNoteRead legacyChangeNoteRead, Change.Id changeId, ObjectReader reader, ObjectId noteId, PatchLineComment.Status status) { super(reader, noteId); - this.noteUtil = noteUtil; + this.legacyChangeNoteRead = legacyChangeNoteRead; + this.noteJson = noteJson; this.changeId = changeId; this.status = status; } @@ -65,7 +68,7 @@ class ChangeRevisionNote extends RevisionNote { p.value = offset; if (isJson(raw, p.value)) { - RevisionNoteData data = parseJson(noteUtil, raw, p.value); + RevisionNoteData data = parseJson(noteJson, raw, p.value); if (status == PatchLineComment.Status.PUBLISHED) { pushCert = data.pushCert; } else { @@ -80,7 +83,7 @@ class ChangeRevisionNote extends RevisionNote { } else { pushCert = null; } - List comments = noteUtil.parseNote(raw, p, changeId); + List comments = legacyChangeNoteRead.parseNote(raw, p, changeId); comments.forEach(c -> c.legacyFormat = true); return comments; } @@ -89,7 +92,7 @@ class ChangeRevisionNote extends RevisionNote { return raw[offset] == '{' || raw[offset] == '['; } - private RevisionNoteData parseJson(ChangeNoteUtil noteUtil, byte[] raw, int offset) + private RevisionNoteData parseJson(ChangeNoteJson noteUtil, byte[] raw, int offset) throws IOException { try (InputStream is = new ByteArrayInputStream(raw, offset, raw.length - offset); Reader r = new InputStreamReader(is, UTF_8)) { diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java index ccc5859baa..445f7a049b 100644 --- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -40,7 +40,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WI import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TAG; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_WORK_IN_PROGRESS; -import static com.google.gerrit.server.notedb.ChangeNoteUtil.sanitizeFooter; +import static com.google.gerrit.server.notedb.NoteDbUtil.sanitizeFooter; import static java.util.Comparator.comparing; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; @@ -527,7 +527,13 @@ public class ChangeUpdate extends AbstractChangeUpdate { for (Map.Entry e : builders.entrySet()) { ObjectId data = - inserter.insert(OBJ_BLOB, e.getValue().build(noteUtil, noteUtil.getWriteJson())); + inserter.insert( + OBJ_BLOB, + e.getValue() + .build( + noteUtil.getChangeNoteJson(), + noteUtil.getLegacyChangeNoteWrite(), + noteUtil.getChangeNoteJson().getWriteJson())); rnm.noteMap.set(ObjectId.fromString(e.getKey().get()), data); } @@ -555,7 +561,12 @@ public class ChangeUpdate extends AbstractChangeUpdate { // Even though reading from changes might not be enabled, we need to // parse any existing revision notes so we can merge them. return RevisionNoteMap.parse( - noteUtil, getId(), rw.getObjectReader(), noteMap, PatchLineComment.Status.PUBLISHED); + noteUtil.getChangeNoteJson(), + noteUtil.getLegacyChangeNoteRead(), + getId(), + rw.getObjectReader(), + noteMap, + PatchLineComment.Status.PUBLISHED); } private void checkComments( @@ -738,7 +749,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } if (readOnlyUntil != null) { - addFooter(msg, FOOTER_READ_ONLY_UNTIL, ChangeNoteUtil.formatTime(serverIdent, readOnlyUntil)); + addFooter(msg, FOOTER_READ_ONLY_UNTIL, NoteDbUtil.formatTime(serverIdent, readOnlyUntil)); } if (isPrivate != null) { diff --git a/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java b/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java index b3907ebdf6..9a8c1305f1 100644 --- a/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java +++ b/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java @@ -133,9 +133,14 @@ public class DeleteCommentRewriter implements NoteDbRewriter { */ @VisibleForTesting public static Map getPublishedComments( - ChangeNoteUtil noteUtil, Change.Id changeId, ObjectReader reader, NoteMap noteMap) + ChangeNoteJson changeNoteJson, + LegacyChangeNoteRead legacyChangeNoteRead, + Change.Id changeId, + ObjectReader reader, + NoteMap noteMap) throws IOException, ConfigInvalidException { - return RevisionNoteMap.parse(noteUtil, changeId, reader, noteMap, PUBLISHED) + return RevisionNoteMap.parse( + changeNoteJson, legacyChangeNoteRead, changeId, reader, noteMap, PUBLISHED) .revisionNotes .values() .stream() @@ -143,6 +148,16 @@ public class DeleteCommentRewriter implements NoteDbRewriter { .collect(toMap(c -> c.key.uuid, Function.identity())); } + public static Map getPublishedComments( + ChangeNoteUtil noteUtil, Change.Id changeId, ObjectReader reader, NoteMap noteMap) + throws IOException, ConfigInvalidException { + return getPublishedComments( + noteUtil.getChangeNoteJson(), + noteUtil.getLegacyChangeNoteRead(), + changeId, + reader, + noteMap); + } /** * Gets the comments put in by the current commit. The message of the target comment will be * replaced by the new message. @@ -205,7 +220,12 @@ public class DeleteCommentRewriter implements NoteDbRewriter { throws IOException, ConfigInvalidException { RevisionNoteMap revNotesMap = RevisionNoteMap.parse( - noteUtil, changeId, reader, NoteMap.read(reader, parentCommit), PUBLISHED); + noteUtil.getChangeNoteJson(), + noteUtil.getLegacyChangeNoteRead(), + changeId, + reader, + NoteMap.read(reader, parentCommit), + PUBLISHED); RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(revNotesMap); for (Comment c : putInComments) { @@ -219,7 +239,13 @@ public class DeleteCommentRewriter implements NoteDbRewriter { Map builders = cache.getBuilders(); for (Map.Entry entry : builders.entrySet()) { ObjectId objectId = ObjectId.fromString(entry.getKey().get()); - byte[] data = entry.getValue().build(noteUtil, noteUtil.getWriteJson()); + byte[] data = + entry + .getValue() + .build( + noteUtil.getChangeNoteJson(), + noteUtil.getLegacyChangeNoteWrite(), + noteUtil.getChangeNoteJson().getWriteJson()); if (data.length == 0) { revNotesMap.noteMap.remove(objectId); } else { diff --git a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java index 008f31fbbc..f66665c8a7 100644 --- a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java +++ b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java @@ -155,7 +155,8 @@ public class DraftCommentNotes extends AbstractChangeNotes { ObjectReader reader = handle.walk().getObjectReader(); revisionNoteMap = RevisionNoteMap.parse( - args.noteUtil, + args.changeNoteJson, + args.legacyChangeNoteRead, getChangeId(), reader, NoteMap.read(reader, tipCommit), diff --git a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java new file mode 100644 index 0000000000..819c8ac730 --- /dev/null +++ b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java @@ -0,0 +1,402 @@ +// Copyright (C) 2018 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.gerrit.server.notedb.ChangeNotes.parseException; +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Comment; +import com.google.gerrit.reviewdb.client.Comment.Key; +import com.google.gerrit.reviewdb.client.CommentRange; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.server.config.GerritServerId; +import com.google.inject.Inject; +import java.sql.Timestamp; +import java.text.ParseException; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Locale; +import java.util.Set; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.util.GitDateParser; +import org.eclipse.jgit.util.MutableInteger; +import org.eclipse.jgit.util.QuotedString; +import org.eclipse.jgit.util.RawParseUtils; + +public class LegacyChangeNoteRead { + private final String serverId; + + @Inject + public LegacyChangeNoteRead(@GerritServerId String serverId) { + this.serverId = serverId; + } + + public Account.Id parseIdent(PersonIdent ident, Change.Id changeId) + throws ConfigInvalidException { + return NoteDbUtil.parseIdent(ident, serverId) + .orElseThrow( + () -> + parseException( + changeId, + "invalid identity, expected @%s: %s", + serverId, + ident.getEmailAddress())); + } + + private static boolean match(byte[] note, MutableInteger p, byte[] expected) { + int m = RawParseUtils.match(note, p.value, expected); + return m == p.value + expected.length; + } + + public List parseNote(byte[] note, MutableInteger p, Change.Id changeId) + throws ConfigInvalidException { + if (p.value >= note.length) { + return ImmutableList.of(); + } + Set seen = new HashSet<>(); + List result = new ArrayList<>(); + int sizeOfNote = note.length; + byte[] psb = ChangeNoteUtil.PATCH_SET.getBytes(UTF_8); + byte[] bpsb = ChangeNoteUtil.BASE_PATCH_SET.getBytes(UTF_8); + byte[] bpn = ChangeNoteUtil.PARENT_NUMBER.getBytes(UTF_8); + + RevId revId = new RevId(parseStringField(note, p, changeId, ChangeNoteUtil.REVISION)); + String fileName = null; + PatchSet.Id psId = null; + boolean isForBase = false; + Integer parentNumber = null; + + while (p.value < sizeOfNote) { + boolean matchPs = match(note, p, psb); + boolean matchBase = match(note, p, bpsb); + if (matchPs) { + fileName = null; + psId = parsePsId(note, p, changeId, ChangeNoteUtil.PATCH_SET); + isForBase = false; + } else if (matchBase) { + fileName = null; + psId = parsePsId(note, p, changeId, ChangeNoteUtil.BASE_PATCH_SET); + isForBase = true; + if (match(note, p, bpn)) { + parentNumber = parseParentNumber(note, p, changeId); + } + } else if (psId == null) { + throw parseException( + changeId, + "missing %s or %s header", + ChangeNoteUtil.PATCH_SET, + ChangeNoteUtil.BASE_PATCH_SET); + } + + Comment c = parseComment(note, p, fileName, psId, revId, isForBase, parentNumber); + fileName = c.key.filename; + if (!seen.add(c.key)) { + throw parseException(changeId, "multiple comments for %s in note", c.key); + } + result.add(c); + } + return result; + } + + private Comment parseComment( + byte[] note, + MutableInteger curr, + String currentFileName, + PatchSet.Id psId, + RevId revId, + boolean isForBase, + Integer parentNumber) + throws ConfigInvalidException { + Change.Id changeId = psId.getParentKey(); + + // Check if there is a new file. + boolean newFile = + (RawParseUtils.match(note, curr.value, ChangeNoteUtil.FILE.getBytes(UTF_8))) != -1; + if (newFile) { + // If so, parse the new file name. + currentFileName = parseFilename(note, curr, changeId); + } else if (currentFileName == null) { + throw parseException(changeId, "could not parse %s", ChangeNoteUtil.FILE); + } + + CommentRange range = parseCommentRange(note, curr); + if (range == null) { + throw parseException(changeId, "could not parse %s", ChangeNoteUtil.COMMENT_RANGE); + } + + Timestamp commentTime = parseTimestamp(note, curr, changeId); + Account.Id aId = parseAuthor(note, curr, changeId, ChangeNoteUtil.AUTHOR); + boolean hasRealAuthor = + (RawParseUtils.match(note, curr.value, ChangeNoteUtil.REAL_AUTHOR.getBytes(UTF_8))) != -1; + Account.Id raId = null; + if (hasRealAuthor) { + raId = parseAuthor(note, curr, changeId, ChangeNoteUtil.REAL_AUTHOR); + } + + boolean hasParent = + (RawParseUtils.match(note, curr.value, ChangeNoteUtil.PARENT.getBytes(UTF_8))) != -1; + String parentUUID = null; + boolean unresolved = false; + if (hasParent) { + parentUUID = parseStringField(note, curr, changeId, ChangeNoteUtil.PARENT); + } + boolean hasUnresolved = + (RawParseUtils.match(note, curr.value, ChangeNoteUtil.UNRESOLVED.getBytes(UTF_8))) != -1; + if (hasUnresolved) { + unresolved = parseBooleanField(note, curr, changeId, ChangeNoteUtil.UNRESOLVED); + } + + String uuid = parseStringField(note, curr, changeId, ChangeNoteUtil.UUID); + + boolean hasTag = + (RawParseUtils.match(note, curr.value, ChangeNoteUtil.TAG.getBytes(UTF_8))) != -1; + String tag = null; + if (hasTag) { + tag = parseStringField(note, curr, changeId, ChangeNoteUtil.TAG); + } + + int commentLength = parseCommentLength(note, curr, changeId); + + String message = RawParseUtils.decode(UTF_8, note, curr.value, curr.value + commentLength); + checkResult(message, "message contents", changeId); + + Comment c = + new Comment( + new Comment.Key(uuid, currentFileName, psId.get()), + aId, + commentTime, + isForBase ? (short) (parentNumber == null ? 0 : -parentNumber) : (short) 1, + message, + serverId, + unresolved); + c.lineNbr = range.getEndLine(); + c.parentUuid = parentUUID; + c.tag = tag; + c.setRevId(revId); + if (raId != null) { + c.setRealAuthor(raId); + } + + if (range.getStartCharacter() != -1) { + c.setRange(range); + } + + curr.value = RawParseUtils.nextLF(note, curr.value + commentLength); + curr.value = RawParseUtils.nextLF(note, curr.value); + return c; + } + + private static String parseStringField( + byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) + throws ConfigInvalidException { + int endOfLine = RawParseUtils.nextLF(note, curr.value); + checkHeaderLineFormat(note, curr, fieldName, changeId); + int startOfField = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; + curr.value = endOfLine; + return RawParseUtils.decode(UTF_8, note, startOfField, endOfLine - 1); + } + + /** + * @return a comment range. If the comment range line in the note only has one number, we return a + * CommentRange with that one number as the end line and the other fields as -1. If the + * comment range line in the note contains a whole comment range, then we return a + * CommentRange with all fields set. If the line is not correctly formatted, return null. + */ + private static CommentRange parseCommentRange(byte[] note, MutableInteger ptr) { + CommentRange range = new CommentRange(-1, -1, -1, -1); + + int last = ptr.value; + int startLine = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (ptr.value == last) { + return null; + } else if (note[ptr.value] == '\n') { + range.setEndLine(startLine); + ptr.value += 1; + return range; + } else if (note[ptr.value] == ':') { + range.setStartLine(startLine); + ptr.value += 1; + } else { + return null; + } + + last = ptr.value; + int startChar = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (ptr.value == last) { + return null; + } else if (note[ptr.value] == '-') { + range.setStartCharacter(startChar); + ptr.value += 1; + } else { + return null; + } + + last = ptr.value; + int endLine = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (ptr.value == last) { + return null; + } else if (note[ptr.value] == ':') { + range.setEndLine(endLine); + ptr.value += 1; + } else { + return null; + } + + last = ptr.value; + int endChar = RawParseUtils.parseBase10(note, ptr.value, ptr); + if (ptr.value == last) { + return null; + } else if (note[ptr.value] == '\n') { + range.setEndCharacter(endChar); + ptr.value += 1; + } else { + return null; + } + return range; + } + + private static PatchSet.Id parsePsId( + byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) + throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, fieldName, changeId); + int startOfPsId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; + MutableInteger i = new MutableInteger(); + int patchSetId = RawParseUtils.parseBase10(note, startOfPsId, i); + int endOfLine = RawParseUtils.nextLF(note, curr.value); + if (i.value != endOfLine - 1) { + throw parseException(changeId, "could not parse %s", fieldName); + } + checkResult(patchSetId, "patchset id", changeId); + curr.value = endOfLine; + return new PatchSet.Id(changeId, patchSetId); + } + + private static Integer parseParentNumber(byte[] note, MutableInteger curr, Change.Id changeId) + throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, ChangeNoteUtil.PARENT_NUMBER, changeId); + + int start = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; + MutableInteger i = new MutableInteger(); + int parentNumber = RawParseUtils.parseBase10(note, start, i); + int endOfLine = RawParseUtils.nextLF(note, curr.value); + if (i.value != endOfLine - 1) { + throw parseException(changeId, "could not parse %s", ChangeNoteUtil.PARENT_NUMBER); + } + checkResult(parentNumber, "parent number", changeId); + curr.value = endOfLine; + return Integer.valueOf(parentNumber); + } + + private static String parseFilename(byte[] note, MutableInteger curr, Change.Id changeId) + throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, ChangeNoteUtil.FILE, changeId); + int startOfFileName = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; + int endOfLine = RawParseUtils.nextLF(note, curr.value); + curr.value = endOfLine; + curr.value = RawParseUtils.nextLF(note, curr.value); + return QuotedString.GIT_PATH.dequote( + RawParseUtils.decode(UTF_8, note, startOfFileName, endOfLine - 1)); + } + + private static Timestamp parseTimestamp(byte[] note, MutableInteger curr, Change.Id changeId) + throws ConfigInvalidException { + int endOfLine = RawParseUtils.nextLF(note, curr.value); + Timestamp commentTime; + String dateString = RawParseUtils.decode(UTF_8, note, curr.value, endOfLine - 1); + try { + commentTime = new Timestamp(GitDateParser.parse(dateString, null, Locale.US).getTime()); + } catch (ParseException e) { + throw new ConfigInvalidException("could not parse comment timestamp", e); + } + curr.value = endOfLine; + return checkResult(commentTime, "comment timestamp", changeId); + } + + private Account.Id parseAuthor( + byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) + throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, fieldName, changeId); + int startOfAccountId = RawParseUtils.endOfFooterLineKey(note, curr.value) + 2; + PersonIdent ident = RawParseUtils.parsePersonIdent(note, startOfAccountId); + Account.Id aId = parseIdent(ident, changeId); + curr.value = RawParseUtils.nextLF(note, curr.value); + return checkResult(aId, fieldName, changeId); + } + + private static int parseCommentLength(byte[] note, MutableInteger curr, Change.Id changeId) + throws ConfigInvalidException { + checkHeaderLineFormat(note, curr, ChangeNoteUtil.LENGTH, changeId); + int startOfLength = RawParseUtils.endOfFooterLineKey(note, curr.value) + 1; + MutableInteger i = new MutableInteger(); + i.value = startOfLength; + int commentLength = RawParseUtils.parseBase10(note, startOfLength, i); + if (i.value == startOfLength) { + throw parseException(changeId, "could not parse %s", ChangeNoteUtil.LENGTH); + } + int endOfLine = RawParseUtils.nextLF(note, curr.value); + if (i.value != endOfLine - 1) { + throw parseException(changeId, "could not parse %s", ChangeNoteUtil.LENGTH); + } + curr.value = endOfLine; + return checkResult(commentLength, "comment length", changeId); + } + + private boolean parseBooleanField( + byte[] note, MutableInteger curr, Change.Id changeId, String fieldName) + throws ConfigInvalidException { + String str = parseStringField(note, curr, changeId, fieldName); + if ("true".equalsIgnoreCase(str)) { + return true; + } else if ("false".equalsIgnoreCase(str)) { + return false; + } + throw parseException(changeId, "invalid boolean for %s: %s", fieldName, str); + } + + private static T checkResult(T o, String fieldName, Change.Id changeId) + throws ConfigInvalidException { + if (o == null) { + throw parseException(changeId, "could not parse %s", fieldName); + } + return o; + } + + private static int checkResult(int i, String fieldName, Change.Id changeId) + throws ConfigInvalidException { + if (i <= 0) { + throw parseException(changeId, "could not parse %s", fieldName); + } + return i; + } + + private static void checkHeaderLineFormat( + byte[] note, MutableInteger curr, String fieldName, Change.Id changeId) + throws ConfigInvalidException { + boolean correct = RawParseUtils.match(note, curr.value, fieldName.getBytes(UTF_8)) != -1; + int p = curr.value + fieldName.length(); + correct &= (p < note.length && note[p] == ':'); + p++; + correct &= (p < note.length && note[p] == ' '); + if (!correct) { + throw parseException(changeId, "could not parse %s", fieldName); + } + } +} diff --git a/java/com/google/gerrit/server/notedb/LegacyChangeNoteWrite.java b/java/com/google/gerrit/server/notedb/LegacyChangeNoteWrite.java new file mode 100644 index 0000000000..1cf0c7c5c6 --- /dev/null +++ b/java/com/google/gerrit/server/notedb/LegacyChangeNoteWrite.java @@ -0,0 +1,206 @@ +// Copyright (C) 2018 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.gerrit.server.CommentsUtil.COMMENT_ORDER; +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ListMultimap; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Comment; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.account.AccountCache; +import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.config.GerritServerId; +import com.google.inject.Inject; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.PrintWriter; +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Date; +import java.util.List; +import java.util.Optional; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.util.QuotedString; + +public class LegacyChangeNoteWrite { + + private final AccountCache accountCache; + private final PersonIdent serverIdent; + private final String serverId; + + @Inject + public LegacyChangeNoteWrite( + AccountCache accountCache, + @GerritPersonIdent PersonIdent serverIdent, + @GerritServerId String serverId) { + this.accountCache = accountCache; + this.serverIdent = serverIdent; + this.serverId = serverId; + } + + public PersonIdent newIdent(Account.Id authorId, Date when, PersonIdent serverIdent) { + Optional author = accountCache.get(authorId).map(AccountState::getAccount); + return new PersonIdent( + author.map(Account::getName).orElseGet(() -> Account.getName(authorId)), + authorId.get() + "@" + serverId, + when, + serverIdent.getTimeZone()); + } + + @VisibleForTesting + public PersonIdent newIdent(Account author, Date when, PersonIdent serverIdent) { + return new PersonIdent( + author.getName(), author.getId().get() + "@" + serverId, when, serverIdent.getTimeZone()); + } + + public String getServerId() { + return serverId; + } + + private void appendHeaderField(PrintWriter writer, String field, String value) { + writer.print(field); + writer.print(": "); + writer.print(value); + writer.print('\n'); + } + + /** + * Build a note that contains the metadata for and the contents of all of the comments in the + * given comments. + * + * @param comments Comments to be written to the output stream, keyed by patch set ID; multiple + * patch sets are allowed since base revisions may be shared across patch sets. All of the + * comments must share the same RevId, and all the comments for a given patch set must have + * the same side. + * @param out output stream to write to. + */ + void buildNote(ListMultimap comments, OutputStream out) { + if (comments.isEmpty()) { + return; + } + + List psIds = new ArrayList<>(comments.keySet()); + Collections.sort(psIds); + + OutputStreamWriter streamWriter = new OutputStreamWriter(out, UTF_8); + try (PrintWriter writer = new PrintWriter(streamWriter)) { + String revId = comments.values().iterator().next().revId; + appendHeaderField(writer, ChangeNoteUtil.REVISION, revId); + + for (int psId : psIds) { + List psComments = COMMENT_ORDER.sortedCopy(comments.get(psId)); + Comment first = psComments.get(0); + + short side = first.side; + appendHeaderField( + writer, + side <= 0 ? ChangeNoteUtil.BASE_PATCH_SET : ChangeNoteUtil.PATCH_SET, + Integer.toString(psId)); + if (side < 0) { + appendHeaderField(writer, ChangeNoteUtil.PARENT_NUMBER, Integer.toString(-side)); + } + + String currentFilename = null; + + for (Comment c : psComments) { + checkArgument( + revId.equals(c.revId), + "All comments being added must have all the same RevId. The " + + "comment below does not have the same RevId as the others " + + "(%s).\n%s", + revId, + c); + checkArgument( + side == c.side, + "All comments being added must all have the same side. The " + + "comment below does not have the same side as the others " + + "(%s).\n%s", + side, + c); + String commentFilename = QuotedString.GIT_PATH.quote(c.key.filename); + + if (!commentFilename.equals(currentFilename)) { + currentFilename = commentFilename; + writer.print("File: "); + writer.print(commentFilename); + writer.print("\n\n"); + } + + appendOneComment(writer, c); + } + } + } + } + + private void appendOneComment(PrintWriter writer, Comment c) { + // The CommentRange field for a comment is allowed to be null. If it is + // null, then in the first line, we simply use the line number field for a + // comment instead. If it isn't null, we write the comment range itself. + Comment.Range range = c.range; + if (range != null) { + writer.print(range.startLine); + writer.print(':'); + writer.print(range.startChar); + writer.print('-'); + writer.print(range.endLine); + writer.print(':'); + writer.print(range.endChar); + } else { + writer.print(c.lineNbr); + } + writer.print("\n"); + + writer.print(NoteDbUtil.formatTime(serverIdent, c.writtenOn)); + writer.print("\n"); + + appendIdent(writer, ChangeNoteUtil.AUTHOR, c.author.getId(), c.writtenOn); + if (!c.getRealAuthor().equals(c.author)) { + appendIdent(writer, ChangeNoteUtil.REAL_AUTHOR, c.getRealAuthor().getId(), c.writtenOn); + } + + String parent = c.parentUuid; + if (parent != null) { + appendHeaderField(writer, ChangeNoteUtil.PARENT, parent); + } + + appendHeaderField(writer, ChangeNoteUtil.UNRESOLVED, Boolean.toString(c.unresolved)); + appendHeaderField(writer, ChangeNoteUtil.UUID, c.key.uuid); + + if (c.tag != null) { + appendHeaderField(writer, ChangeNoteUtil.TAG, c.tag); + } + + byte[] messageBytes = c.message.getBytes(UTF_8); + appendHeaderField(writer, ChangeNoteUtil.LENGTH, Integer.toString(messageBytes.length)); + + writer.print(c.message); + writer.print("\n\n"); + } + + private void appendIdent(PrintWriter writer, String header, Account.Id id, Timestamp ts) { + PersonIdent ident = newIdent(id, ts, serverIdent); + StringBuilder name = new StringBuilder(); + PersonIdent.appendSanitized(name, ident.getName()); + name.append(" <"); + PersonIdent.appendSanitized(name, ident.getEmailAddress()); + name.append('>'); + appendHeaderField(writer, header, name.toString()); + } +} diff --git a/java/com/google/gerrit/server/notedb/NoteDbUtil.java b/java/com/google/gerrit/server/notedb/NoteDbUtil.java index 59c4c62dd3..21fada8173 100644 --- a/java/com/google/gerrit/server/notedb/NoteDbUtil.java +++ b/java/com/google/gerrit/server/notedb/NoteDbUtil.java @@ -14,12 +14,21 @@ package com.google.gerrit.server.notedb; +import com.google.common.base.CharMatcher; import com.google.common.primitives.Ints; import com.google.gerrit.reviewdb.client.Account; +import java.sql.Timestamp; import java.util.Optional; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.util.GitDateFormatter; +import org.eclipse.jgit.util.GitDateFormatter.Format; public class NoteDbUtil { + + /** + * 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) { String email = ident.getEmailAddress(); int at = email.indexOf('@'); @@ -36,4 +45,24 @@ public class NoteDbUtil { } private NoteDbUtil() {} + + public static String formatTime(PersonIdent ident, Timestamp t) { + GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT); + // TODO(dborowitz): Use a ThreadLocal or use Joda. + PersonIdent newIdent = new PersonIdent(ident, t); + return dateFormatter.formatDate(newIdent); + } + + private static final CharMatcher INVALID_FOOTER_CHARS = CharMatcher.anyOf("\r\n\0"); + + static String sanitizeFooter(String value) { + // Remove characters that would confuse JGit's footer parser if they were + // included in footer values, for example by splitting the footer block into + // multiple paragraphs. + // + // One painful example: RevCommit#getShorMessage() might return a message + // containing "\r\r", which RevCommit#getFooterLines() will treat as an + // empty paragraph for the purposes of footer parsing. + return INVALID_FOOTER_CHARS.trimAndCollapseFrom(value, ' '); + } } diff --git a/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java b/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java index b341ea8a41..8bf286d6fb 100644 --- a/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java +++ b/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java @@ -83,11 +83,17 @@ class RevisionNoteBuilder { } public byte[] build(ChangeNoteUtil noteUtil, boolean writeJson) throws IOException { + return build(noteUtil.getChangeNoteJson(), noteUtil.getLegacyChangeNoteWrite(), writeJson); + } + + public byte[] build( + ChangeNoteJson changeNoteJson, LegacyChangeNoteWrite legacyChangeNoteWrite, boolean writeJson) + throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); if (writeJson) { - buildNoteJson(noteUtil, out); + buildNoteJson(changeNoteJson, out); } else { - buildNoteLegacy(noteUtil, out); + buildNoteLegacy(legacyChangeNoteWrite, out); } return out.toByteArray(); } @@ -122,7 +128,7 @@ class RevisionNoteBuilder { return all; } - private void buildNoteJson(ChangeNoteUtil noteUtil, OutputStream out) throws IOException { + private void buildNoteJson(ChangeNoteJson noteUtil, OutputStream out) throws IOException { ListMultimap comments = buildCommentMap(); if (comments.isEmpty() && pushCert == null) { return; @@ -137,7 +143,8 @@ class RevisionNoteBuilder { } } - private void buildNoteLegacy(ChangeNoteUtil noteUtil, OutputStream out) throws IOException { + private void buildNoteLegacy(LegacyChangeNoteWrite noteUtil, OutputStream out) + throws IOException { if (pushCert != null) { byte[] certBytes = pushCert.getBytes(UTF_8); out.write(certBytes, 0, trimTrailingNewlines(certBytes)); diff --git a/java/com/google/gerrit/server/notedb/RevisionNoteMap.java b/java/com/google/gerrit/server/notedb/RevisionNoteMap.java index aa82d1ad0c..17a061a8f2 100644 --- a/java/com/google/gerrit/server/notedb/RevisionNoteMap.java +++ b/java/com/google/gerrit/server/notedb/RevisionNoteMap.java @@ -32,7 +32,8 @@ class RevisionNoteMap> { final ImmutableMap revisionNotes; static RevisionNoteMap parse( - ChangeNoteUtil noteUtil, + ChangeNoteJson noteJson, + LegacyChangeNoteRead legacyChangeNoteRead, Change.Id changeId, ObjectReader reader, NoteMap noteMap, @@ -41,7 +42,8 @@ class RevisionNoteMap> { Map result = new HashMap<>(); for (Note note : noteMap) { ChangeRevisionNote rn = - new ChangeRevisionNote(noteUtil, changeId, reader, note.getData(), status); + new ChangeRevisionNote( + noteJson, legacyChangeNoteRead, changeId, reader, note.getData(), status); rn.parse(); result.put(new RevId(note.name()), rn); } @@ -49,12 +51,12 @@ class RevisionNoteMap> { } static RevisionNoteMap parseRobotComments( - ChangeNoteUtil noteUtil, ObjectReader reader, NoteMap noteMap) + ChangeNoteJson changeNoteJson, ObjectReader reader, NoteMap noteMap) throws ConfigInvalidException, IOException { Map result = new HashMap<>(); for (Note note : noteMap) { RobotCommentsRevisionNote rn = - new RobotCommentsRevisionNote(noteUtil, reader, note.getData()); + new RobotCommentsRevisionNote(changeNoteJson, reader, note.getData()); rn.parse(); result.put(new RevId(note.name()), rn); } diff --git a/java/com/google/gerrit/server/notedb/RobotCommentNotes.java b/java/com/google/gerrit/server/notedb/RobotCommentNotes.java index 99d9615506..7eb3a54100 100644 --- a/java/com/google/gerrit/server/notedb/RobotCommentNotes.java +++ b/java/com/google/gerrit/server/notedb/RobotCommentNotes.java @@ -89,7 +89,8 @@ public class RobotCommentNotes extends AbstractChangeNotes { RevCommit tipCommit = handle.walk().parseCommit(metaId); ObjectReader reader = handle.walk().getObjectReader(); revisionNoteMap = - RevisionNoteMap.parseRobotComments(args.noteUtil, reader, NoteMap.read(reader, tipCommit)); + RevisionNoteMap.parseRobotComments( + args.changeNoteJson, reader, NoteMap.read(reader, tipCommit)); ListMultimap cs = MultimapBuilder.hashKeys().arrayListValues().build(); for (RobotCommentsRevisionNote rn : revisionNoteMap.revisionNotes.values()) { for (RobotComment c : rn.getComments()) { diff --git a/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java b/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java index c28125f408..3a0d595f7e 100644 --- a/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java +++ b/java/com/google/gerrit/server/notedb/RobotCommentUpdate.java @@ -202,7 +202,8 @@ public class RobotCommentUpdate extends AbstractChangeUpdate { } // Even though reading from changes might not be enabled, we need to // parse any existing revision notes so we can merge them. - return RevisionNoteMap.parseRobotComments(noteUtil, rw.getObjectReader(), noteMap); + return RevisionNoteMap.parseRobotComments( + noteUtil.getChangeNoteJson(), rw.getObjectReader(), noteMap); } @Override diff --git a/java/com/google/gerrit/server/notedb/RobotCommentsRevisionNote.java b/java/com/google/gerrit/server/notedb/RobotCommentsRevisionNote.java index aa229ab43a..6c3cc8627e 100644 --- a/java/com/google/gerrit/server/notedb/RobotCommentsRevisionNote.java +++ b/java/com/google/gerrit/server/notedb/RobotCommentsRevisionNote.java @@ -27,9 +27,9 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; public class RobotCommentsRevisionNote extends RevisionNote { - private final ChangeNoteUtil noteUtil; + private final ChangeNoteJson noteUtil; - RobotCommentsRevisionNote(ChangeNoteUtil noteUtil, ObjectReader reader, ObjectId noteId) { + RobotCommentsRevisionNote(ChangeNoteJson noteUtil, ObjectReader reader, ObjectId noteId) { super(reader, noteId); this.noteUtil = noteUtil; } diff --git a/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java index 92a878c801..3a0bfc16f4 100644 --- a/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java +++ b/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java @@ -546,7 +546,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { if (id == null) { return new PersonIdent(serverIdent, events.getWhen()); } - return changeNoteUtil.newIdent(id, events.getWhen(), serverIdent); + return changeNoteUtil.getLegacyChangeNoteWrite().newIdent(id, events.getWhen(), serverIdent); } private List getHashtagsEvents(Change change, NoteDbUpdateManager manager) @@ -564,7 +564,10 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { for (RevCommit commit : rw) { Account.Id authorId; try { - authorId = changeNoteUtil.parseIdent(commit.getAuthorIdent(), change.getId()); + authorId = + changeNoteUtil + .getLegacyChangeNoteRead() + .parseIdent(commit.getAuthorIdent(), change.getId()); } catch (ConfigInvalidException e) { continue; // Corrupt data, no valid hashtags in this commit. } diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 882996ebfd..b85e2f2fc8 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -2676,7 +2676,9 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(commitPatchSetCreation.getShortMessage()).isEqualTo("Create patch set 2"); PersonIdent expectedAuthor = - changeNoteUtil.newIdent(getAccount(admin.id), c.updated, serverIdent.get()); + changeNoteUtil + .getLegacyChangeNoteWrite() + .newIdent(getAccount(admin.id), c.updated, serverIdent.get()); assertThat(commitPatchSetCreation.getAuthorIdent()).isEqualTo(expectedAuthor); assertThat(commitPatchSetCreation.getCommitterIdent()) .isEqualTo(new PersonIdent(serverIdent.get(), c.updated)); @@ -2684,7 +2686,10 @@ public class ChangeIT extends AbstractDaemonTest { RevCommit commitChangeCreation = rw.parseCommit(commitPatchSetCreation.getParent(0)); assertThat(commitChangeCreation.getShortMessage()).isEqualTo("Create change"); - expectedAuthor = changeNoteUtil.newIdent(getAccount(admin.id), c.created, serverIdent.get()); + expectedAuthor = + changeNoteUtil + .getLegacyChangeNoteWrite() + .newIdent(getAccount(admin.id), c.created, serverIdent.get()); assertThat(commitChangeCreation.getAuthorIdent()).isEqualTo(expectedAuthor); assertThat(commitChangeCreation.getCommitterIdent()) .isEqualTo(new PersonIdent(serverIdent.get(), c.created)); diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 45294fbc60..1de9d29e75 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -433,7 +433,9 @@ public class RefAdvertisementIT extends AbstractDaemonTest { if (notesMigration.commitChangeWrites()) { PersonIdent committer = serverIdent.get(); PersonIdent author = - noteUtil.newIdent(getAccount(admin.getId()), committer.getWhen(), committer); + noteUtil + .getLegacyChangeNoteWrite() + .newIdent(getAccount(admin.getId()), committer.getWhen(), committer); tr.branch(RefNames.changeMetaRef(c3.getId())) .commit() .author(author) diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index e510e25989..3e9b90c548 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -273,7 +273,9 @@ public class CreateChangeIT extends AbstractDaemonTest { assertThat(commit.getShortMessage()).isEqualTo("Create change"); PersonIdent expectedAuthor = - changeNoteUtil.newIdent(getAccount(admin.id), c.created, serverIdent.get()); + changeNoteUtil + .getLegacyChangeNoteWrite() + .newIdent(getAccount(admin.id), c.created, serverIdent.get()); assertThat(commit.getAuthorIdent()).isEqualTo(expectedAuthor); assertThat(commit.getCommitterIdent()) diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index e53b9979f6..9621c16270 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -950,7 +950,7 @@ public class CommentsIT extends AbstractDaemonTest { @Test public void jsonCommentHasLegacyFormatFalse() throws Exception { assume().that(notesMigration.readChanges()).isTrue(); - assertThat(noteUtil.getWriteJson()).isTrue(); + assertThat(noteUtil.getChangeNoteJson().getWriteJson()).isTrue(); PushOneCommit.Result result = createChange(); Change.Id changeId = result.getChange().getId(); diff --git a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 29c043af65..ea44bd70c4 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -902,7 +902,9 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { } PersonIdent committer = serverIdent.get(); PersonIdent author = - noteUtil.newIdent(getAccount(admin.getId()), committer.getWhen(), committer); + noteUtil + .getLegacyChangeNoteWrite() + .newIdent(getAccount(admin.getId()), committer.getWhen(), committer); serverSideTestRepo .branch(RefNames.changeMetaRef(id)) .commit() diff --git a/javatests/com/google/gerrit/acceptance/server/change/LegacyCommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/LegacyCommentsIT.java index a3a0339fbb..e029e7aec4 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/LegacyCommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/LegacyCommentsIT.java @@ -53,7 +53,7 @@ public class LegacyCommentsIT extends AbstractDaemonTest { @Test public void legacyCommentHasLegacyFormatTrue() throws Exception { assume().that(notesMigration.readChanges()).isTrue(); - assertThat(noteUtil.getWriteJson()).isFalse(); + assertThat(noteUtil.getChangeNoteJson().getWriteJson()).isFalse(); PushOneCommit.Result result = createChange(); Change.Id changeId = result.getChange().getId(); diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java index 95d96b5f57..b7ce7bc90e 100644 --- a/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java +++ b/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java @@ -17,8 +17,8 @@ package com.google.gerrit.acceptance.server.notedb; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static com.google.gerrit.server.notedb.ChangeNoteUtil.formatTime; import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB; +import static com.google.gerrit.server.notedb.NoteDbUtil.formatTime; import static java.util.concurrent.TimeUnit.DAYS; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java index b8f544a203..de964d86bb 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java @@ -498,7 +498,11 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { private RevCommit writeCommit(String body) throws Exception { ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class); return writeCommit( - body, noteUtil.newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent), false); + body, + noteUtil + .getLegacyChangeNoteWrite() + .newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent), + false); } private RevCommit writeCommit(String body, PersonIdent author) throws Exception { @@ -509,7 +513,9 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class); return writeCommit( body, - noteUtil.newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent), + noteUtil + .getLegacyChangeNoteWrite() + .newIdent(changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent), initWorkInProgress); } @@ -555,7 +561,9 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { private ChangeNotesParser newParser(ObjectId tip) throws Exception { walk.reset(); - ChangeNoteUtil noteUtil = injector.getInstance(ChangeNoteUtil.class); - return new ChangeNotesParser(newChange().getId(), tip, walk, noteUtil, args.metrics); + ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class); + LegacyChangeNoteRead reader = injector.getInstance(LegacyChangeNoteRead.class); + return new ChangeNotesParser( + newChange().getId(), tip, walk, changeNoteJson, reader, args.metrics); } } diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java index 9d387049ea..4b553fd833 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -77,6 +77,8 @@ import org.junit.Test; public class ChangeNotesTest extends AbstractChangeNotesTest { @Inject private DraftCommentNotes.Factory draftNotesFactory; + @Inject private ChangeNoteJson changeNoteJson; + @Inject private LegacyChangeNoteRead legacyChangeNoteRead; @Inject private ChangeNoteUtil noteUtil; @Inject private @GerritServerId String serverId; @@ -1195,7 +1197,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "File: a.txt\n" + "\n" + "1:2-3:4\n" - + ChangeNoteUtil.formatTime(serverIdent, ts) + + NoteDbUtil.formatTime(serverIdent, ts) + "\n" + "Author: Change Owner <1@gerrit>\n" + "Unresolved: false\n" @@ -1288,7 +1290,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) { ChangeNotesParser notesWithComments = - new ChangeNotesParser(c.getId(), commitWithComments.copy(), rw, noteUtil, args.metrics); + new ChangeNotesParser( + c.getId(), + commitWithComments.copy(), + rw, + changeNoteJson, + legacyChangeNoteRead, + args.metrics); ChangeNotesState state = notesWithComments.parseAll(); assertThat(state.approvals()).isEmpty(); assertThat(state.publishedComments()).hasSize(1); @@ -1296,7 +1304,14 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) { ChangeNotesParser notesWithApprovals = - new ChangeNotesParser(c.getId(), commitWithApprovals.copy(), rw, noteUtil, args.metrics); + new ChangeNotesParser( + c.getId(), + commitWithApprovals.copy(), + rw, + changeNoteJson, + legacyChangeNoteRead, + args.metrics); + ChangeNotesState state = notesWithApprovals.parseAll(); assertThat(state.approvals()).hasSize(1); assertThat(state.publishedComments()).hasSize(1); @@ -1666,7 +1681,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "File: file1\n" + "\n" + "1:1-2:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time1) + + NoteDbUtil.formatTime(serverIdent, time1) + "\n" + "Author: Other Account <2@gerrit>\n" + "Unresolved: false\n" @@ -1675,7 +1690,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "comment 1\n" + "\n" + "2:1-3:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time2) + + NoteDbUtil.formatTime(serverIdent, time2) + "\n" + "Author: Other Account <2@gerrit>\n" + "Unresolved: false\n" @@ -1686,7 +1701,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "File: file2\n" + "\n" + "3:0-4:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time3) + + NoteDbUtil.formatTime(serverIdent, time3) + "\n" + "Author: Other Account <2@gerrit>\n" + "Unresolved: false\n" @@ -1766,7 +1781,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "File: file1\n" + "\n" + "1:1-2:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time1) + + NoteDbUtil.formatTime(serverIdent, time1) + "\n" + "Author: Other Account <2@gerrit>\n" + "Unresolved: false\n" @@ -1775,7 +1790,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "comment 1\n" + "\n" + "2:1-3:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time2) + + NoteDbUtil.formatTime(serverIdent, time2) + "\n" + "Author: Other Account <2@gerrit>\n" + "Unresolved: false\n" @@ -1854,7 +1869,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "File: file1\n" + "\n" + "1:1-2:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time1) + + NoteDbUtil.formatTime(serverIdent, time1) + "\n" + "Author: Other Account <2@gerrit>\n" + "Unresolved: false\n" @@ -1863,7 +1878,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "comment 1\n" + "\n" + "1:1-2:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time2) + + NoteDbUtil.formatTime(serverIdent, time2) + "\n" + "Author: Other Account <2@gerrit>\n" + "Parent: uuid1\n" @@ -1951,7 +1966,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { byte[] bytes = walk.getObjectReader().open(note.getData(), Constants.OBJ_BLOB).getBytes(); String noteString = new String(bytes, UTF_8); - String timeStr = ChangeNoteUtil.formatTime(serverIdent, time); + String timeStr = NoteDbUtil.formatTime(serverIdent, time); if (!testJson()) { assertThat(noteString) @@ -2048,7 +2063,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { + "File: file\n" + "\n" + "1:1-2:1\n" - + ChangeNoteUtil.formatTime(serverIdent, time) + + NoteDbUtil.formatTime(serverIdent, time) + "\n" + "Author: Other Account <2@gerrit>\n" + "Real-author: Change Owner <1@gerrit>\n" @@ -2103,7 +2118,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { byte[] bytes = walk.getObjectReader().open(note.getData(), Constants.OBJ_BLOB).getBytes(); String noteString = new String(bytes, UTF_8); - String timeStr = ChangeNoteUtil.formatTime(serverIdent, time); + String timeStr = NoteDbUtil.formatTime(serverIdent, time); if (!testJson()) { assertThat(noteString) @@ -3511,7 +3526,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { } private boolean testJson() { - return noteUtil.getWriteJson(); + return noteUtil.getChangeNoteJson().getWriteJson(); } private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception { diff --git a/javatests/com/google/gerrit/server/notedb/CommentTimestampAdapterTest.java b/javatests/com/google/gerrit/server/notedb/CommentTimestampAdapterTest.java index aa37d511c2..e7d89562e4 100644 --- a/javatests/com/google/gerrit/server/notedb/CommentTimestampAdapterTest.java +++ b/javatests/com/google/gerrit/server/notedb/CommentTimestampAdapterTest.java @@ -68,7 +68,7 @@ public class CommentTimestampAdapterTest { // Match ChangeNoteUtil#gson as of 4e1f02db913d91f2988f559048e513e6093a1bce legacyGson = new GsonBuilder().setPrettyPrinting().create(); - gson = ChangeNoteUtil.newGson(); + gson = ChangeNoteJson.newGson(); } @After