diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java index dbd1abb293..0acf20e7f3 100644 --- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java +++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java @@ -50,7 +50,6 @@ public abstract class AbstractChangeNotes { public final ChangeNoteJson changeNoteJson; public final GitRepositoryManager repoManager; public final AllUsersName allUsers; - public final LegacyChangeNoteRead legacyChangeNoteRead; public final NoteDbMetrics metrics; public final String serverId; @@ -64,14 +63,12 @@ public abstract class AbstractChangeNotes { GitRepositoryManager repoManager, AllUsersName allUsers, ChangeNoteJson changeNoteJson, - LegacyChangeNoteRead legacyChangeNoteRead, NoteDbMetrics metrics, Provider cache, @GerritServerId String serverId) { this.failOnLoadForTest = new AtomicBoolean(); this.repoManager = repoManager; this.allUsers = allUsers; - this.legacyChangeNoteRead = legacyChangeNoteRead; this.changeNoteJson = changeNoteJson; this.metrics = metrics; this.cache = cache; diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index 507fc60c46..877022edeb 100644 --- a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -264,12 +264,7 @@ 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.getChangeNoteJson(), - noteUtil.getLegacyChangeNoteRead(), - getId(), - rw.getObjectReader(), - noteMap, - Comment.Status.DRAFT); + noteUtil.getChangeNoteJson(), rw.getObjectReader(), noteMap, Comment.Status.DRAFT); } @Override diff --git a/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java index 7aa3bc839d..b221ef51b3 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java +++ b/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java @@ -63,22 +63,13 @@ public class ChangeNoteUtil { static final String UNRESOLVED = "Unresolved"; static final String TAG = FOOTER_TAG.getName(); - private final LegacyChangeNoteRead legacyChangeNoteRead; private final ChangeNoteJson changeNoteJson; private final String serverId; @Inject - public ChangeNoteUtil( - ChangeNoteJson changeNoteJson, - LegacyChangeNoteRead legacyChangeNoteRead, - @GerritServerId String serverId) { + public ChangeNoteUtil(ChangeNoteJson changeNoteJson, @GerritServerId String serverId) { this.serverId = serverId; this.changeNoteJson = changeNoteJson; - this.legacyChangeNoteRead = legacyChangeNoteRead; - } - - public LegacyChangeNoteRead getLegacyChangeNoteRead() { - return legacyChangeNoteRead; } public ChangeNoteJson getChangeNoteJson() { diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java index 8f5fefec59..366008d1e3 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java @@ -354,12 +354,7 @@ public class ChangeNotesCache { "Load change notes for change %s of project %s", key.changeId(), key.project()); ChangeNotesParser parser = new ChangeNotesParser( - key.changeId(), - key.id(), - walkSupplier.get(), - args.changeNoteJson, - args.legacyChangeNoteRead, - args.metrics); + key.changeId(), key.id(), walkSupplier.get(), args.changeNoteJson, 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 5973718211..9c45aaf007 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -102,7 +102,6 @@ class ChangeNotesParser { // Private final members initialized in the constructor. private final ChangeNoteJson changeNoteJson; - private final LegacyChangeNoteRead legacyChangeNoteRead; private final NoteDbMetrics metrics; private final Change.Id id; @@ -155,13 +154,11 @@ class ChangeNotesParser { ObjectId tip, ChangeNotesRevWalk walk, ChangeNoteJson changeNoteJson, - LegacyChangeNoteRead legacyChangeNoteRead, NoteDbMetrics metrics) { this.id = changeId; this.tip = tip; this.walk = walk; this.changeNoteJson = changeNoteJson; - this.legacyChangeNoteRead = legacyChangeNoteRead; this.metrics = metrics; approvals = new LinkedHashMap<>(); bufferedApprovals = new ArrayList<>(); @@ -443,7 +440,7 @@ class ChangeNotesParser { return effectiveAccountId; } PersonIdent ident = RawParseUtils.parsePersonIdent(realUser); - return legacyChangeNoteRead.parseIdent(ident, id); + return parseIdent(ident); } private String parseTopic(ChangeNotesCommit commit) throws ConfigInvalidException { @@ -575,7 +572,7 @@ class ChangeNotesParser { parsedAssignee = Optional.empty(); } else { PersonIdent ident = RawParseUtils.parsePersonIdent(assigneeValue); - parsedAssignee = Optional.ofNullable(legacyChangeNoteRead.parseIdent(ident, id)); + parsedAssignee = Optional.ofNullable(parseIdent(ident)); } assigneeUpdates.add(AssigneeStatusUpdate.create(ts, ownerId, parsedAssignee)); } @@ -708,12 +705,7 @@ class ChangeNotesParser { ChangeNotesCommit tipCommit = walk.parseCommit(tip); revisionNoteMap = RevisionNoteMap.parse( - changeNoteJson, - legacyChangeNoteRead, - id, - reader, - NoteMap.read(reader, tipCommit), - Comment.Status.PUBLISHED); + changeNoteJson, reader, NoteMap.read(reader, tipCommit), Comment.Status.PUBLISHED); Map rns = revisionNoteMap.revisionNotes; for (Map.Entry e : rns.entrySet()) { @@ -772,7 +764,7 @@ class ChangeNotesParser { labelVoteStr = line.substring(0, s); PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); checkFooter(ident != null, FOOTER_LABEL, line); - effectiveAccountId = legacyChangeNoteRead.parseIdent(ident, id); + effectiveAccountId = parseIdent(ident); } else { labelVoteStr = line; effectiveAccountId = committerId; @@ -811,7 +803,7 @@ class ChangeNotesParser { label = line.substring(1, s); PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1)); checkFooter(ident != null, FOOTER_LABEL, line); - effectiveAccountId = legacyChangeNoteRead.parseIdent(ident, id); + effectiveAccountId = parseIdent(ident); } else { label = line.substring(1); effectiveAccountId = committerId; @@ -870,7 +862,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 = legacyChangeNoteRead.parseIdent(ident, id); + label.appliedBy = parseIdent(ident); } else { label.label = line.substring(c + 2); } @@ -886,7 +878,7 @@ class ChangeNotesParser { if (a.getName().equals(c.getName()) && a.getEmailAddress().equals(c.getEmailAddress())) { return null; } - return legacyChangeNoteRead.parseIdent(commit.getAuthorIdent(), id); + return parseIdent(commit.getAuthorIdent()); } private void parseReviewer(Timestamp ts, ReviewerStateInternal state, String line) @@ -895,7 +887,7 @@ class ChangeNotesParser { if (ident == null) { throw invalidFooter(state.getFooterKey(), line); } - Account.Id accountId = legacyChangeNoteRead.parseIdent(ident, id); + Account.Id accountId = parseIdent(ident); reviewerUpdates.add(ReviewerStatusUpdate.create(ts, ownerId, accountId, state)); if (!reviewers.containsRow(accountId)) { reviewers.put(accountId, state, ts); @@ -1095,4 +1087,10 @@ class ChangeNotesParser { private ConfigInvalidException parseException(String fmt, Object... args) { return ChangeNotes.parseException(id, fmt, args); } + + private Account.Id parseIdent(PersonIdent ident) throws ConfigInvalidException { + return NoteDbUtil.parseIdent(ident) + .orElseThrow( + () -> parseException("cannot retrieve account id: %s", ident.getEmailAddress())); + } } diff --git a/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java b/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java index 249b4c2165..b6443f15b9 100644 --- a/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java +++ b/java/com/google/gerrit/server/notedb/ChangeRevisionNote.java @@ -16,8 +16,6 @@ package com.google.gerrit.server.notedb; import static java.nio.charset.StandardCharsets.UTF_8; -import com.google.common.primitives.Bytes; -import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Comment; import java.io.ByteArrayInputStream; import java.io.IOException; @@ -29,30 +27,16 @@ import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.util.MutableInteger; -import org.eclipse.jgit.util.RawParseUtils; class ChangeRevisionNote extends RevisionNote { - private static final byte[] CERT_HEADER = "certificate version ".getBytes(UTF_8); - // See org.eclipse.jgit.transport.PushCertificateParser.END_SIGNATURE - private static final byte[] END_SIGNATURE = "-----END PGP SIGNATURE-----\n".getBytes(UTF_8); - private final ChangeNoteJson noteJson; - private final LegacyChangeNoteRead legacyChangeNoteRead; - private final Change.Id changeId; private final Comment.Status status; private String pushCert; ChangeRevisionNote( - ChangeNoteJson noteJson, - LegacyChangeNoteRead legacyChangeNoteRead, - Change.Id changeId, - ObjectReader reader, - ObjectId noteId, - Comment.Status status) { + ChangeNoteJson noteJson, ObjectReader reader, ObjectId noteId, Comment.Status status) { super(reader, noteId); - this.legacyChangeNoteRead = legacyChangeNoteRead; this.noteJson = noteJson; - this.changeId = changeId; this.status = status; } @@ -66,29 +50,13 @@ class ChangeRevisionNote extends RevisionNote { MutableInteger p = new MutableInteger(); p.value = offset; - if (isJson(raw, p.value)) { - RevisionNoteData data = parseJson(noteJson, raw, p.value); - if (status == Comment.Status.PUBLISHED) { - pushCert = data.pushCert; - } else { - pushCert = null; - } - return data.comments; - } - + RevisionNoteData data = parseJson(noteJson, raw, p.value); if (status == Comment.Status.PUBLISHED) { - pushCert = parsePushCert(changeId, raw, p); - trimLeadingEmptyLines(raw, p); + pushCert = data.pushCert; } else { pushCert = null; } - List comments = legacyChangeNoteRead.parseNote(raw, p, changeId); - comments.forEach(c -> c.legacyFormat = true); - return comments; - } - - static boolean isJson(byte[] raw, int offset) { - return raw[offset] == '{' || raw[offset] == '['; + return data.comments; } private RevisionNoteData parseJson(ChangeNoteJson noteUtil, byte[] raw, int offset) @@ -98,18 +66,4 @@ class ChangeRevisionNote extends RevisionNote { return noteUtil.getGson().fromJson(r, RevisionNoteData.class); } } - - private static String parsePushCert(Change.Id changeId, byte[] bytes, MutableInteger p) - throws ConfigInvalidException { - if (RawParseUtils.match(bytes, p.value, CERT_HEADER) < 0) { - return null; - } - int end = Bytes.indexOf(bytes, END_SIGNATURE); - if (end < 0) { - throw ChangeNotes.parseException(changeId, "invalid push certificate in note"); - } - int start = p.value; - p.value = end + END_SIGNATURE.length; - return new String(bytes, start, p.value, UTF_8); - } } diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 243a196fbb..c0cd173428 100644 --- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -461,12 +461,7 @@ 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.getChangeNoteJson(), - noteUtil.getLegacyChangeNoteRead(), - getId(), - rw.getObjectReader(), - noteMap, - Comment.Status.PUBLISHED); + noteUtil.getChangeNoteJson(), rw.getObjectReader(), noteMap, Comment.Status.PUBLISHED); } private void checkComments( diff --git a/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java b/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java index ef82389abd..9c8b369ee8 100644 --- a/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java +++ b/java/com/google/gerrit/server/notedb/DeleteCommentRewriter.java @@ -95,13 +95,13 @@ public class DeleteCommentRewriter implements NoteDbRewriter { ObjectReader reader = revWalk.getObjectReader(); RevCommit newTipCommit = revWalk.next(); // The first commit will not be rewritten. Map parentComments = - getPublishedComments(noteUtil, changeId, reader, NoteMap.read(reader, newTipCommit)); + getPublishedComments(noteUtil, reader, NoteMap.read(reader, newTipCommit)); boolean rewrite = false; RevCommit originalCommit; while ((originalCommit = revWalk.next()) != null) { NoteMap noteMap = NoteMap.read(reader, originalCommit); - Map currComments = getPublishedComments(noteUtil, changeId, reader, noteMap); + Map currComments = getPublishedComments(noteUtil, reader, noteMap); if (!rewrite && currComments.containsKey(uuid)) { rewrite = true; @@ -131,28 +131,18 @@ public class DeleteCommentRewriter implements NoteDbRewriter { */ @VisibleForTesting public static Map getPublishedComments( - ChangeNoteJson changeNoteJson, - LegacyChangeNoteRead legacyChangeNoteRead, - Change.Id changeId, - ObjectReader reader, - NoteMap noteMap) + ChangeNoteJson changeNoteJson, ObjectReader reader, NoteMap noteMap) throws IOException, ConfigInvalidException { - return RevisionNoteMap.parse( - changeNoteJson, legacyChangeNoteRead, changeId, reader, noteMap, Status.PUBLISHED) - .revisionNotes.values().stream() + return RevisionNoteMap.parse(changeNoteJson, reader, noteMap, Status.PUBLISHED).revisionNotes + .values().stream() .flatMap(n -> n.getEntities().stream()) .collect(toMap(c -> c.key.uuid, Function.identity())); } public static Map getPublishedComments( - ChangeNoteUtil noteUtil, Change.Id changeId, ObjectReader reader, NoteMap noteMap) + ChangeNoteUtil noteUtil, ObjectReader reader, NoteMap noteMap) throws IOException, ConfigInvalidException { - return getPublishedComments( - noteUtil.getChangeNoteJson(), - noteUtil.getLegacyChangeNoteRead(), - changeId, - reader, - noteMap); + return getPublishedComments(noteUtil.getChangeNoteJson(), reader, noteMap); } /** * Gets the comments put in by the current commit. The message of the target comment will be @@ -215,8 +205,6 @@ public class DeleteCommentRewriter implements NoteDbRewriter { RevisionNoteMap revNotesMap = RevisionNoteMap.parse( noteUtil.getChangeNoteJson(), - noteUtil.getLegacyChangeNoteRead(), - changeId, reader, NoteMap.read(reader, parentCommit), Status.PUBLISHED); diff --git a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java index 09e5af7159..3966396961 100644 --- a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java +++ b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java @@ -120,12 +120,7 @@ public class DraftCommentNotes extends AbstractChangeNotes { ObjectReader reader = handle.walk().getObjectReader(); revisionNoteMap = RevisionNoteMap.parse( - args.changeNoteJson, - args.legacyChangeNoteRead, - getChangeId(), - reader, - NoteMap.read(reader, tipCommit), - Comment.Status.DRAFT); + args.changeNoteJson, reader, NoteMap.read(reader, tipCommit), Comment.Status.DRAFT); ListMultimap cs = MultimapBuilder.hashKeys().arrayListValues().build(); for (ChangeRevisionNote rn : revisionNoteMap.revisionNotes.values()) { for (Comment c : rn.getEntities()) { diff --git a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java deleted file mode 100644 index e663149c51..0000000000 --- a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java +++ /dev/null @@ -1,399 +0,0 @@ -// 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.entities.Account; -import com.google.gerrit.entities.Change; -import com.google.gerrit.entities.Comment; -import com.google.gerrit.entities.CommentRange; -import com.google.gerrit.entities.PatchSet; -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.ObjectId; -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) - .orElseThrow( - () -> - parseException( - changeId, "cannot retrieve account id: %s", 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); - - ObjectId commitId = - ObjectId.fromString(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, commitId, 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, - ObjectId commitId, - boolean isForBase, - Integer parentNumber) - throws ConfigInvalidException { - Change.Id changeId = psId.changeId(); - - // 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.setCommitId(commitId); - 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 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 deleted file mode 100644 index a31a68bb3c..0000000000 --- a/java/com/google/gerrit/server/notedb/LegacyChangeNoteWrite.java +++ /dev/null @@ -1,198 +0,0 @@ -// 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.common.collect.ImmutableList.toImmutableList; -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.ImmutableList; -import com.google.common.collect.ListMultimap; -import com.google.gerrit.common.UsedAt; -import com.google.gerrit.entities.Account; -import com.google.gerrit.entities.Comment; -import com.google.gerrit.server.GerritPersonIdent; -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.Date; -import java.util.List; -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.PersonIdent; -import org.eclipse.jgit.util.QuotedString; - -public class LegacyChangeNoteWrite { - - private final PersonIdent serverIdent; - private final String serverId; - - @Inject - public LegacyChangeNoteWrite( - @GerritPersonIdent PersonIdent serverIdent, @GerritServerId String serverId) { - this.serverIdent = serverIdent; - this.serverId = serverId; - } - - public PersonIdent newIdent(Account.Id authorId, Date when, PersonIdent serverIdent) { - return new PersonIdent( - authorId.toString(), authorId.get() + "@" + serverId, when, serverIdent.getTimeZone()); - } - - @VisibleForTesting - public PersonIdent newIdent(Account author, Date when, PersonIdent serverIdent) { - return new PersonIdent( - author.toString(), author.id().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 commitId, and all the comments for a given patch set must have - * the same side. - * @param out output stream to write to. - */ - @UsedAt(UsedAt.Project.GOOGLE) - public void buildNote(ListMultimap comments, OutputStream out) { - if (comments.isEmpty()) { - return; - } - - ImmutableList psIds = comments.keySet().stream().sorted().collect(toImmutableList()); - - OutputStreamWriter streamWriter = new OutputStreamWriter(out, UTF_8); - try (PrintWriter writer = new PrintWriter(streamWriter)) { - ObjectId commitId = comments.values().iterator().next().getCommitId(); - String commitName = commitId.name(); - appendHeaderField(writer, ChangeNoteUtil.REVISION, commitName); - - 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( - commitId.equals(c.getCommitId()), - "All comments being added must have all the same commitId. The " - + "comment below does not have the same commitId as the others " - + "(%s).\n%s", - commitId, - 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/RevisionNoteMap.java b/java/com/google/gerrit/server/notedb/RevisionNoteMap.java index 08f7a6db3f..3e1bad1951 100644 --- a/java/com/google/gerrit/server/notedb/RevisionNoteMap.java +++ b/java/com/google/gerrit/server/notedb/RevisionNoteMap.java @@ -15,7 +15,6 @@ package com.google.gerrit.server.notedb; import com.google.common.collect.ImmutableMap; -import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Comment; import java.io.IOException; import java.util.HashMap; @@ -31,18 +30,11 @@ class RevisionNoteMap> { final ImmutableMap revisionNotes; static RevisionNoteMap parse( - ChangeNoteJson noteJson, - LegacyChangeNoteRead legacyChangeNoteRead, - Change.Id changeId, - ObjectReader reader, - NoteMap noteMap, - Comment.Status status) + ChangeNoteJson noteJson, ObjectReader reader, NoteMap noteMap, Comment.Status status) throws ConfigInvalidException, IOException { Map result = new HashMap<>(); for (Note note : noteMap) { - ChangeRevisionNote rn = - new ChangeRevisionNote( - noteJson, legacyChangeNoteRead, changeId, reader, note.getData(), status); + ChangeRevisionNote rn = new ChangeRevisionNote(noteJson, reader, note.getData(), status); rn.parse(); result.put(note.copy(), rn); } diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index b22d319311..bf2b91539f 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -1058,10 +1058,10 @@ public class CommentsIT extends AbstractDaemonTest { Map commentMapBefore = DeleteCommentRewriter.getPublishedComments( - noteUtil, changeId, reader, NoteMap.read(reader, commitBefore)); + noteUtil, reader, NoteMap.read(reader, commitBefore)); Map commentMapAfter = DeleteCommentRewriter.getPublishedComments( - noteUtil, changeId, reader, NoteMap.read(reader, commitAfter)); + noteUtil, reader, NoteMap.read(reader, commitAfter)); if (commentMapBefore.containsKey(targetCommentUuid)) { assertThat(commentMapAfter).containsKey(targetCommentUuid); diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java index 7882518f9a..013939a400 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java @@ -571,8 +571,6 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { private ChangeNotesParser newParser(ObjectId tip) throws Exception { walk.reset(); ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class); - LegacyChangeNoteRead reader = injector.getInstance(LegacyChangeNoteRead.class); - return new ChangeNotesParser( - newChange().getId(), tip, walk, changeNoteJson, reader, args.metrics); + return new ChangeNotesParser(newChange().getId(), tip, walk, changeNoteJson, args.metrics); } } diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java index 960039e0f7..5993206eba 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -74,7 +74,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { @Inject private DraftCommentNotes.Factory draftNotesFactory; @Inject private ChangeNoteJson changeNoteJson; - @Inject private LegacyChangeNoteRead legacyChangeNoteRead; @Inject private @GerritServerId String serverId; @@ -1297,12 +1296,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) { ChangeNotesParser notesWithComments = new ChangeNotesParser( - c.getId(), - commitWithComments.copy(), - rw, - changeNoteJson, - legacyChangeNoteRead, - args.metrics); + c.getId(), commitWithComments.copy(), rw, changeNoteJson, args.metrics); ChangeNotesState state = notesWithComments.parseAll(); assertThat(state.approvals()).isEmpty(); assertThat(state.publishedComments()).hasSize(1); @@ -1311,12 +1305,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) { ChangeNotesParser notesWithApprovals = new ChangeNotesParser( - c.getId(), - commitWithApprovals.copy(), - rw, - changeNoteJson, - legacyChangeNoteRead, - args.metrics); + c.getId(), commitWithApprovals.copy(), rw, changeNoteJson, args.metrics); ChangeNotesState state = notesWithApprovals.parseAll(); assertThat(state.approvals()).hasSize(1);