Skip to content
Permalink
Browse files

22: Wrong lines for comments

Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Aug 7, 2019
1 parent e8d32c7 commit 2698709516a4417b0b745365dd20d22ce6533ddf
@@ -426,6 +426,72 @@ void reviewContext(TestInfo testInfo) throws IOException {
}
}

@Test
void multipleReviewContexts(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var archiveFolder = new TemporaryDirectory();
var listServer = new TestMailmanServer()) {
var author = credentials.getHostedRepository();
var archive = credentials.getHostedRepository();
var listAddress = EmailAddress.parse(listServer.createList("test"));
var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.host().getCurrentUserDetails().id());
var from = EmailAddress.from("test", "test@test.mail");
var mlBot = new MailingListBridgeBot(from, author, archive, listAddress, Set.of(), listServer.getArchive(),
listServer.getSMTP(),
archive, "webrev", Path.of("test"),
URIBuilder.base("http://www.test.test/").build(),
Set.of(), Map.of());

// Populate the projects repository
var reviewFile = Path.of("reviewfile.txt");
var localRepo = CheckableRepository.init(tempFolder.path(), author.getRepositoryType(), reviewFile);
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.getUrl(), "master", true);
localRepo.push(masterHash, archive.getUrl(), "webrev", true);
var initialHash = CheckableRepository.appendAndCommit(localRepo,
"Line 0.1\nLine 0.2\nLine 0.3\nLine 0.4\n" +
"Line 1\nLine 2\nLine 3\nLine 4\n" +
"Line 5\nLine 6\nLine 7\nLine 8\n" +
"Line 8.1\nLine 8.2\nLine 8.3\nLine 8.4\n" +
"Line 9\nLine 10\nLine 11\nLine 12\n" +
"Line 13\nLine 14\nLine 15\nLine 16\n");
localRepo.push(initialHash, author.getUrl(), "master");

// Make a change with a corresponding PR
var current = Files.readString(localRepo.root().resolve(reviewFile), StandardCharsets.UTF_8);
var updated = current.replaceAll("Line 2", "Line 2 edit\nLine 2.5");
updated = updated.replaceAll("Line 13", "Line 12.5\nLine 13 edit");
Files.writeString(localRepo.root().resolve(reviewFile), updated, StandardCharsets.UTF_8);
var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.getUrl(), "edit", true);
var pr = credentials.createPullRequest(archive, "master", "edit", "This is a pull request");
pr.setBody("This is now ready");
TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// Make file specific comments
pr.addReviewComment(masterHash, editHash, reviewFile.toString(), 7, "Review comment");
pr.addReviewComment(masterHash, editHash, reviewFile.toString(), 24, "Another review comment");

TestBotRunner.runPeriodicItems(mlBot);
listServer.processIncoming();

// The archive should only contain context around line 2 and 20
Repository.materialize(archiveFolder.path(), archive.getUrl(), "master");
assertTrue(archiveContains(archiveFolder.path(), "reviewfile.txt line 7"));
assertTrue(archiveContains(archiveFolder.path(), "^> 6: Line 1$"));
assertTrue(archiveContains(archiveFolder.path(), "^> 7: Line 2 edit$"));
assertFalse(archiveContains(archiveFolder.path(), "Line 3"));

assertTrue(archiveContains(archiveFolder.path(), "reviewfile.txt line 24"));
assertTrue(archiveContains(archiveFolder.path(), "^> 23: Line 12.5$"));
assertTrue(archiveContains(archiveFolder.path(), "^> 24: Line 13 edit$"));
assertFalse(archiveContains(archiveFolder.path(), "^Line 15"));
}
}

@Test
void filterComments(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
@@ -23,7 +23,7 @@
package org.openjdk.skara.host.github;

import org.openjdk.skara.host.*;
import org.openjdk.skara.host.network.*;
import org.openjdk.skara.host.network.RestRequest;
import org.openjdk.skara.json.*;
import org.openjdk.skara.vcs.Hash;

@@ -111,14 +111,14 @@ public void addReview(Review.Verdict verdict, String body) {
.execute();
}

private ReviewComment parseReviewComment(ReviewComment parent, JSONObject json) {
private ReviewComment parseReviewComment(ReviewComment parent, JSONObject json, PositionMapper diff) {
var author = host.parseUserDetails(json);
var threadId = parent == null ? json.get("id").toString() : parent.threadId();
var comment = new ReviewComment(parent,
threadId,
new Hash(json.get("commit_id").asString()),
json.get("path").asString(),
json.get("original_position").asInt(), // FIXME: This is not the line
diff.positionToLine(json.get("path").asString(), json.get("original_position").asInt()),
json.get("id").toString(),
json.get("body").asString(),
author,
@@ -129,30 +129,45 @@ private ReviewComment parseReviewComment(ReviewComment parent, JSONObject json)

@Override
public ReviewComment addReviewComment(Hash base, Hash hash, String path, int line, String body) {
var rawDiff = request.get("pulls/" + json.get("number").toString())
.header("Accept", "application/vnd.github.v3.diff")
.executeUnparsed();
var diff = PositionMapper.parse(rawDiff);

var query = JSON.object()
.put("body", body)
.put("commit_id", hash.hex())
.put("path", path)
.put("position", line);
.put("position", diff.lineToPosition(path, line));
var response = request.post("pulls/" + json.get("number").toString() + "/comments")
.body(query)
.execute();
return parseReviewComment(null, response.asObject());
return parseReviewComment(null, response.asObject(), diff);
}

@Override
public ReviewComment addReviewCommentReply(ReviewComment parent, String body) {
var rawDiff = request.get("pulls/" + json.get("number").toString())
.header("Accept", "application/vnd.github.v3.diff")
.executeUnparsed();
var diff = PositionMapper.parse(rawDiff);

var query = JSON.object()
.put("body", body)
.put("in_reply_to", Integer.parseInt(parent.threadId()));
var response = request.post("pulls/" + json.get("number").toString() + "/comments")
.body(query)
.execute();
return parseReviewComment(parent, response.asObject());
return parseReviewComment(parent, response.asObject(), diff);
}

@Override
public List<ReviewComment> getReviewComments() {
var rawDiff = request.get("pulls/" + json.get("number").toString())
.header("Accept", "application/vnd.github.v3.diff")
.executeUnparsed();
var diff = PositionMapper.parse(rawDiff);

var ret = new ArrayList<ReviewComment>();
var reviewComments = request.get("pulls/" + json.get("number").toString() + "/comments").execute().stream()
.map(JSONValue::asObject)
@@ -164,7 +179,7 @@ public ReviewComment addReviewCommentReply(ReviewComment parent, String body) {
if (reviewComment.contains("in_reply_to_id")) {
parent = idToComment.get(reviewComment.get("in_reply_to_id").toString());
}
var comment = parseReviewComment(parent, reviewComment);
var comment = parseReviewComment(parent, reviewComment, diff);
idToComment.put(comment.id(), comment);
ret.add(comment);
}
@@ -0,0 +1,108 @@
/*
* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.host.github;

import java.util.*;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

class PositionMapper {
private static final Pattern filePattern = Pattern.compile("^diff --git a/(.*) b/.*$");
private static final Pattern hunkPattern = Pattern.compile("^@@ -(\\d+)(?:,\\d+)? \\+(\\d+)(?:,\\d+)? @@.*");

private static class PositionOffset {
int position;
int line;
}

private final Map<String, List<PositionOffset>> fileDiffs = new HashMap<>();
private final Logger log = Logger.getLogger("org.openjdk.skara.host.github");

private PositionMapper(List<String> lines) {
int position = 0;
var latestList = new ArrayList<PositionOffset>();

for (var line : lines) {
position++;
var fileMatcher = filePattern.matcher(line);
if (fileMatcher.matches()) {
latestList = new ArrayList<>();
fileDiffs.put(fileMatcher.group(1), latestList);
continue;
}
var hunkMatcher = hunkPattern.matcher(line);
if (hunkMatcher.matches()) {
if (latestList.isEmpty()) {
position = 1;
}
var positionOffset = new PositionOffset();
positionOffset.position = position;
positionOffset.line = Integer.parseInt(hunkMatcher.group(2));
latestList.add(positionOffset);
}
}
}

int positionToLine(String file, int position) {
if (!fileDiffs.containsKey(file)) {
throw new IllegalArgumentException("Unknown file " + file);
}
var positionOffsets = fileDiffs.get(file);
PositionOffset activeOffset = null;
for (var offset : positionOffsets) {
if (offset.position > position) {
break;
}
activeOffset = offset;
}
if (activeOffset == null) {
log.warning("No matching line found (position: " + position + " file: " + file + ")");
return -1;
}
return activeOffset.line + (position - activeOffset.position);
}

int lineToPosition(String file, int line) {
if (!fileDiffs.containsKey(file)) {
throw new IllegalArgumentException("Unknown file " + file);
}
var positionOffsets = fileDiffs.get(file);
PositionOffset activeOffset = null;
for (var offset : positionOffsets) {
if (offset.line > line) {
break;
}
activeOffset = offset;
}
if (activeOffset == null) {
log.warning("No matching position found (line: " + line + " file: " + file + ")");
return -1;
}
return activeOffset.position + (line - activeOffset.line);
}

static PositionMapper parse(String diff) {
return new PositionMapper(diff.lines().collect(Collectors.toList()));
}
}
@@ -63,6 +63,7 @@

private final List<Param> params = new ArrayList<>();
private final List<Param> bodyParams = new ArrayList<>();
private final Map<String, String> headers = new HashMap<>();
private JSONValue body;
private int maxPages;
private ErrorTransform onError;
@@ -143,6 +144,11 @@ public QueryBuilder onError(ErrorTransform errorTransform) {
return this;
}

public QueryBuilder header(String name, String value) {
headers.put(name, value);
return this;
}

public JSONValue execute() {
return RestRequest.this.execute(this);
}
@@ -259,7 +265,8 @@ private JSONValue parseResponse(HttpResponse<String> response) {
}
}

private HttpRequest createRequest(RequestType requestType, String endpoint, JSONValue body, List<QueryBuilder.Param> params) {
private HttpRequest createRequest(RequestType requestType, String endpoint, JSONValue body,
List<QueryBuilder.Param> params, Map<String, String> headers) {
var uriBuilder = URIBuilder.base(apiBase);
if (endpoint != null && !endpoint.isEmpty()) {
uriBuilder = uriBuilder.appendPath(endpoint);
@@ -279,6 +286,7 @@ private HttpRequest createRequest(RequestType requestType, String endpoint, JSON
if (body != null) {
requestBuilder.method(requestType.name(), HttpRequest.BodyPublishers.ofString(body.toString()));
}
headers.forEach(requestBuilder::header);
return requestBuilder.build();
}

@@ -290,7 +298,8 @@ private HttpRequest createRequest(RequestType requestType, String endpoint, JSON
}

private JSONValue execute(QueryBuilder queryBuilder) {
var request = createRequest(queryBuilder.queryType, queryBuilder.endpoint, queryBuilder.composedBody(), queryBuilder.params);
var request = createRequest(queryBuilder.queryType, queryBuilder.endpoint, queryBuilder.composedBody(),
queryBuilder.params, queryBuilder.headers);
var response = sendRequest(request);
var errorTransform = transformBadResponse(response, queryBuilder);
if (errorTransform.isPresent()) {
@@ -331,7 +340,8 @@ private JSONValue execute(QueryBuilder queryBuilder) {
}

private String executeUnparsed(QueryBuilder queryBuilder) {
var request = createRequest(queryBuilder.queryType, queryBuilder.endpoint, queryBuilder.composedBody(), queryBuilder.params);
var request = createRequest(queryBuilder.queryType, queryBuilder.endpoint, queryBuilder.composedBody(),
queryBuilder.params, queryBuilder.headers);
var response = sendRequest(request);
return response.body();
}

0 comments on commit 2698709

Please sign in to comment.