Skip to content
This repository has been archived by the owner on Jan 7, 2021. It is now read-only.

implement resetting of overview comments #162

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/main/java/org/sonar/plugins/stash/StashPluginUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import com.google.common.base.CharMatcher;
import java.io.IOException;
import java.util.Collection;
import java.util.Optional;
import java.util.Properties;
import java.util.stream.Stream;
import org.sonar.api.batch.fs.InputComponent;
import org.sonar.api.batch.fs.InputModule;
import org.sonar.api.batch.postjob.issue.PostJobIssue;
Expand Down Expand Up @@ -72,4 +74,8 @@ public static String removeEnd(String s, String suffix) {
}
return s;
}

public static <T> Stream<T> removeEmpty(Optional<T> v) {
return v.map(Stream::of).orElse(Stream.empty());
}
}
52 changes: 27 additions & 25 deletions src/main/java/org/sonar/plugins/stash/StashRequestFacade.java
Original file line number Diff line number Diff line change
Expand Up @@ -398,39 +398,41 @@ public void resetComments(PullRequestRef pr,
StashUser sonarUser,
StashClient stashClient) {
try {
// Let's call this "diffRep_loop"
for (StashComment comment : diffReport.getComments()) {

// delete comment only if published by the current SQ user
if (sonarUser.getId() != comment.getAuthor().getId()) {
continue;
// Next element in "diffRep_loop"

// comment contains tasks which cannot be deleted => do nothing
} else if (comment.containsPermanentTasks()) {
LOGGER.debug("Comment \"{}\" (path:\"{}\", line:\"{}\")"
+ "CANNOT be deleted because one of its tasks is not deletable.", comment.getId(),
comment.getPath(),
comment.getLine());
continue; // Next element in "diffRep_loop"
}

// delete tasks linked to the current comment
for (StashTask task : comment.getTasks()) {
stashClient.deleteTaskOnComment(task);
}

stashClient.deletePullRequestComment(pr, comment);
}
// FIXME delete tasks on file-wide comments
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Take the required action to fix the issue indicated by this comment. rule

// resetComments(diffReport.getComments(), pr, sonarUser, stashClient);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR This block of commented-out lines of code should be removed. rule

resetComments(stashClient.getPullRequestOverviewComments(pr), pr, sonarUser, stashClient);

LOGGER.info("SonarQube issues reported to Stash by user \"{}\" have been reset",
sonarUser.getName());

} catch (StashClientException e) {
LOGGER.error("Unable to reset comment list", e);
}
}

private void resetComments(Collection<StashComment> comments, PullRequestRef pr, StashUser sonarUser, StashClient stashClient)
throws StashClientException {
for (StashComment comment : comments) {
if (sonarUser.getId() != comment.getAuthor().getId()) {
continue;
}

if (comment.containsPermanentTasks()) {
LOGGER.debug("Comment \"{}\" (path:\"{}\", line:\"{}\")"
+ "CANNOT be deleted because one of its tasks is not deletable.", comment.getId(),
comment.getPath(),
comment.getLine());
continue; // Next element in "diffRep_loop"
}

// delete tasks linked to the current comment
for (StashTask task : comment.getTasks()) {
stashClient.deleteTaskOnComment(task);
}

stashClient.deletePullRequestComment(pr, comment);
}
}

@Override
public String getIssuePath(PostJobIssue issue) {
InputComponent ip = issue.inputComponent();
Expand Down
74 changes: 53 additions & 21 deletions src/main/java/org/sonar/plugins/stash/client/StashClient.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.sonar.plugins.stash.client;

import java.util.Collection;
import java.util.Optional;
import java.util.stream.Collectors;
import org.asynchttpclient.AsyncHttpClient;
import org.asynchttpclient.BoundRequestBuilder;
import org.asynchttpclient.DefaultAsyncHttpClient;
Expand All @@ -19,7 +21,6 @@
import org.sonar.plugins.stash.PeekableInputStream;
import org.sonar.plugins.stash.PluginInfo;
import org.sonar.plugins.stash.PullRequestRef;
import org.sonar.plugins.stash.StashPlugin;
import org.sonar.plugins.stash.StashPlugin.IssueType;
import org.sonar.plugins.stash.StashPluginUtils;
import org.sonar.plugins.stash.exceptions.StashClientException;
Expand Down Expand Up @@ -64,7 +65,7 @@ public class StashClient implements AutoCloseable {
private static final String API_ONE_PR_ALL_COMMENTS = API_ONE_PR + "/comments";
private static final String API_ONE_PR_DIFF = API_ONE_PR + "/diff?withComments=true";
private static final String API_ONE_PR_APPROVAL = API_ONE_PR + "/approve";
private static final String API_ONE_PR_COMMENT_PATH = API_ONE_PR + "/comments?path={4}&start={5,number,#}";
private static final String API_ONE_PR_COMMENT_PATH = API_ONE_PR + "/comments?path={4}";

private static final String API_ONE_PR_ONE_COMMENT = API_ONE_PR_ALL_COMMENTS + "/{4}?version={5}";

Expand Down Expand Up @@ -111,31 +112,29 @@ public void postCommentOnPullRequest(PullRequestRef pr, String report)
postCreate(request, json, MessageFormat.format(COMMENT_POST_ERROR_MESSAGE, pr.repository(), pr.pullRequestId()));
}

public Collection<StashComment> getPullRequestOverviewComments(PullRequestRef pr) throws StashClientException {
return getPaged(
MessageFormat.format(API_ONE_PR + "/activities", baseUrl, pr.project(), pr.repository(), pr.pullRequestId()),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Format specifiers should be used instead of string concatenation. rule

"Error!"
).stream().map(StashCollector::extractCommentFromActivity).flatMap(StashPluginUtils::removeEmpty).collect(Collectors.toList());
}

public StashCommentReport getPullRequestComments(PullRequestRef pr, String path)
throws StashClientException {
StashCommentReport result = new StashCommentReport();

long start = 0;
boolean isLastPage = false;
String url = MessageFormat.format(API_ONE_PR_COMMENT_PATH,
baseUrl,
pr.project(),
pr.repository(),
pr.pullRequestId(),
path
);

while (!isLastPage) {
for (JsonObject jsonComment: getPaged(url,
MessageFormat.format(COMMENT_GET_ERROR_MESSAGE, pr.repository(), pr.pullRequestId()))) {
try {
String request = MessageFormat.format(API_ONE_PR_COMMENT_PATH,
baseUrl,
pr.project(),
pr.repository(),
pr.pullRequestId(),
path,
start);
JsonObject jsonComments = get(request,
MessageFormat.format(COMMENT_GET_ERROR_MESSAGE,
pr.repository(),
pr.pullRequestId()));
result.add(StashCollector.extractComments(jsonComments));

// Stash pagination: check if you get all comments linked to the pull-request
isLastPage = StashCollector.isLastPage(jsonComments);
start = StashCollector.getNextPageStart(jsonComments);
result.add(StashCollector.extractComment(jsonComment));
} catch (StashReportExtractionException e) {
throw new StashClientException(e);
}
Expand Down Expand Up @@ -310,6 +309,10 @@ private JsonObject get(String url, String errorMessage) throws StashClientExcept
return performRequest(httpClient.prepareGet(url), null, HttpURLConnection.HTTP_OK, errorMessage);
}

private Collection<JsonObject> getPaged(String url, String errorMessage) throws StashClientException {
return performPagedRequest(httpClient.prepareGet(url), null, HttpURLConnection.HTTP_OK, errorMessage);
}

private JsonObject post(String url, JsonObject body, String errorMessage) throws StashClientException {
return performRequest(httpClient.preparePost(url), body, HttpURLConnection.HTTP_OK, errorMessage);
}
Expand Down Expand Up @@ -456,4 +459,33 @@ AsyncHttpClient createHttpClient(String sonarQubeVersion) {
.build()
);
}

private Collection<JsonObject> performPagedRequest(BoundRequestBuilder requestBuilder,
JsonObject body,
int expectedStatusCode,
String errorMessage) throws StashClientException {

Collection<JsonObject> result = new ArrayList<>();

boolean doneLastPage = false;
int nextPageStart = 0;
// FIXME size/limit support
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Take the required action to fix the issue indicated by this comment. rule


while (!doneLastPage) {
if (nextPageStart > 0) {
requestBuilder.addQueryParam("start", String.valueOf(nextPageStart));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will add an additional parameter "start" and not replace the previous one. With more activities you will end up with link params:
?start=25&start=50&start50&start-50...
which will at the end fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marekjagielski Good point. thanks!

}

JsonObject res = performRequest(requestBuilder, body, expectedStatusCode, errorMessage);
JsonArray values = ((JsonArray) res.get("values"));
if (values == null) {
throw new StashClientException("Paged response did not include values");
}
values.asCollection(result);
doneLastPage = res.getBooleanOrDefault("isLastPage", true);
nextPageStart = res.getIntegerOrDefault("nextPageStart", -1);
}

return result;
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.sonar.plugins.stash.issue.collector;

import java.math.BigDecimal;
import java.util.Objects;
import java.util.Optional;
import org.json.simple.JsonArray;
import org.json.simple.JsonObject;
import org.sonar.plugins.stash.PullRequestRef;
Expand All @@ -20,9 +22,7 @@ public final class StashCollector {
private static final String AUTHOR = "author";
private static final String VERSION = "version";

private StashCollector() {
// Hiding implicit public constructor with an explicit private one (squid:S1118)
}
private StashCollector() {}

public static StashCommentReport extractComments(JsonObject jsonComments) throws StashReportExtractionException {
StashCommentReport result = new StashCommentReport();
Expand All @@ -41,8 +41,11 @@ public static StashCommentReport extractComments(JsonObject jsonComments) throws
return result;
}

public static StashComment extractComment(JsonObject jsonComment, String path, Long line) {
public static Optional<StashComment> extractCommentFromActivity(JsonObject json) {
return Optional.ofNullable((JsonObject) json.get("comment")).filter(Objects::nonNull).map(j -> StashCollector.extractComment(j, null, null));
}

public static StashComment extractComment(JsonObject jsonComment, String path, Long line) {
long id = getLong(jsonComment, "id");
String message = (String)jsonComment.get("text");

Expand All @@ -51,7 +54,10 @@ public static StashComment extractComment(JsonObject jsonComment, String path, L
JsonObject jsonAuthor = (JsonObject)jsonComment.get(AUTHOR);
StashUser stashUser = extractUser(jsonAuthor);

return new StashComment(id, message, path, line, stashUser, version);
StashComment result = new StashComment(id, message, path, line, stashUser, version);
// FIXME do this at some central place
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Take the required action to fix the issue indicated by this comment. rule

updateCommentTasks(result, (JsonArray) jsonComment.get("tasks"));
return result;
}

public static StashComment extractComment(JsonObject jsonComment) throws StashReportExtractionException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ public void setUp() throws Exception {
comments.add(comment3);

when(diffReport.getComments()).thenReturn(comments);
when(stashClient.getPullRequestOverviewComments(pr)).thenReturn(comments);

stashCommentsReport1 = mock(StashCommentReport.class);
when(stashCommentsReport1.getComments()).thenReturn(comments);
Expand Down Expand Up @@ -713,6 +714,7 @@ public void testResetCommentsWithDifferentStashUsers() throws Exception {
comments.add(comment);

when(diffReport.getComments()).thenReturn(comments);
when(stashClient.getPullRequestOverviewComments(eq(pr))).thenReturn(comments);

myFacade.resetComments(pr, diffReport, stashUser, stashClient);

Expand All @@ -721,7 +723,8 @@ public void testResetCommentsWithDifferentStashUsers() throws Exception {

@Test
public void testResetCommentsWithoutAnyComments() throws Exception {
when(diffReport.getComments()).thenReturn(new ArrayList<StashComment>());
when(diffReport.getComments()).thenReturn(new ArrayList<>());
when(stashClient.getPullRequestOverviewComments(eq(pr))).thenReturn(new ArrayList<>());

myFacade.resetComments(pr, diffReport, stashUser, stashClient);

Expand Down
17 changes: 12 additions & 5 deletions src/test/java/org/sonar/plugins/stash/client/StashClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,7 @@ public void testGetPullRequestCommentsWithNextPage() throws Exception {
+ "\"author\": {\"id\":1, \"name\":\"SonarQube\", \"slug\":\"sonarqube\", \"email\":\"sq@email.com\"}, \"version\": 0}], \"isLastPage\": true}";

wireMock.stubFor(get(
urlPathEqualTo("/rest/api/1.0/projects/Project/repos/Repository/pull-requests/1/comments"))
.withQueryParam("start", equalTo(String.valueOf(0))).willReturn(
urlPathEqualTo("/rest/api/1.0/projects/Project/repos/Repository/pull-requests/1/comments")).willReturn(
aJsonResponse().withStatus(HttpURLConnection.HTTP_OK).withBody(stashJsonComment1)
));

Expand All @@ -177,7 +176,7 @@ public void testGetPullRequestCommentsWithNoNextPage() throws Exception {
"{\"values\": [{\"id\":4321, \"text\":\"message2\", \"anchor\": {\"path\":\"path\", \"line\":10},"
+ "\"author\": {\"id\":1, \"name\":\"SonarQube\", \"slug\":\"sonarqube\", \"email\":\"sq@email.com\"}, \"version\": 0}], \"isLastPage\": true}";

wireMock.stubFor(get(anyUrl()).withQueryParam("start", equalTo(String.valueOf(0))).willReturn(
wireMock.stubFor(get(anyUrl()).willReturn(
aJsonResponse().withStatus(HttpURLConnection.HTTP_OK).withBody(stashJsonComment1)));

wireMock.stubFor(get(anyUrl()).withQueryParam("start", equalTo(String.valueOf(1))).willReturn(
Expand Down Expand Up @@ -371,7 +370,7 @@ public void testPullRequestHugePullRequestId() throws Exception {
// See https://github.com/AmadeusITGroup/sonar-stash/issues/98
int hugePullRequestId = 1234567890;

wireMock.stubFor(any(anyUrl()).willReturn(aJsonResponse()));
wireMock.stubFor(any(anyUrl()).willReturn(aPagedJsonResponse()));

PullRequestRef pr = PullRequestRef.builder()
.setProject("Project")
Expand Down Expand Up @@ -424,7 +423,15 @@ private void addErrorResponse(MappingBuilder mapping, int statusCode) {
}

public static ResponseDefinitionBuilder aJsonResponse() {
return aResponse().withHeader("Content-Type", "application/json").withBody("{}");
return aJsonResponse("{}");
}

private ResponseDefinitionBuilder aPagedJsonResponse() {
return aJsonResponse("{\"values\":[]}");
}

public static ResponseDefinitionBuilder aJsonResponse(String body) {
return aResponse().withHeader("Content-Type", "application/json").withBody(body);
}

public static ResponseDefinitionBuilder aXMLResponse() {
Expand Down