opendev/gerrit
Code Issues Proposed changes

Upgrade gitiles-servlet and blame-cache to 0.2-11

Browse Source 
If a file doesn't exist in a commit we expect the GetBlame REST endpoint
to return an empty BlameInfo list. This works well unless the file
existed once and had been deleted. E.g. if there is a change that
recreates a file and the user clicks on 'SHOW BLAME' on the diff screen
the request to get the blame info for the base commit fails with 500
Internal Server Error. For example the newly added
GetBlameIT#forRecreatedFileFromBase test was failing like this:

com.google.common.util.concurrent.UncheckedExecutionException: java.lang.NullPointerException
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2051)
	at com.google.common.cache.LocalCache.get(LocalCache.java:3953)
	at com.google.common.cache.LocalCache$LocalManualCache.get(LocalCache.java:4873)
	at com.google.gitiles.blame.cache.BlameCacheImpl.get(BlameCacheImpl.java:114)
	at com.google.gerrit.server.restapi.change.GetBlame.blame(GetBlame.java:145)
	at com.google.gerrit.server.restapi.change.GetBlame.apply(GetBlame.java:118)
	at com.google.gerrit.server.api.changes.FileApiImpl$2.get(FileApiImpl.java:149)
	at com.google.gerrit.acceptance.api.revision.GetBlameIT.forRecreatedFileFromBase(GetBlameIT.java:123)
        ...
Caused by: java.lang.NullPointerException
	at com.google.gitiles.blame.cache.BlameCacheImpl.loadRegions(BlameCacheImpl.java:156)
	at com.google.gitiles.blame.cache.BlameCacheImpl.loadBlame(BlameCacheImpl.java:139)
	at com.google.gitiles.blame.cache.BlameCacheImpl.lambda$newLoader$1(BlameCacheImpl.java:103)
	at com.google.common.cache.LocalCache$LocalManualCache$1.load(LocalCache.java:4878)
	at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3529)
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2278)
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2155)
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2045)
	... 48 more

The updated version of blame-cache includes a fix for this.

Likely we want to have more tests for the GetBlame REST endpoint, but
adding those is outside the scope of this change.

Bug: Issue 5082
Change-Id: I11b529b48495e563148040dfb7f56379de13d128
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2019-11-14 15:08:20 +01:00
committed by David Pursehouse
parent beba231e4e
commit 59e3f0b6fa
5 changed files with 185 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
WORKSPACE
View File

@@ -702,7 +702,7 @@ maven_jar(
sha1 = "f7be08ec23c21485b9b5a1cf1654c2ec8c58168d",
)
GITILES_VERS = "0.2-10"
GITILES_VERS = "0.2-11"
GITILES_REPO = GERRIT
@@ -711,14 +711,14 @@ maven_jar(
artifact = "com.google.gitiles:blame-cache:" + GITILES_VERS,
attach_source = False,
repository = GITILES_REPO,
sha1 = "7ee42a4f2a2f88d2768a78c5b6e5c7c9fe79b0fa",
sha1 = "254c1e81184301feccbcd857013c88f4d4ee32d9",
)
maven_jar(
name = "gitiles-servlet",
artifact = "com.google.gitiles:gitiles-servlet:" + GITILES_VERS,
repository = GITILES_REPO,
sha1 = "9b78bd8efab7c161019364bff57e9ab9a2e2a475",
sha1 = "0a3e6cbdb99d42459b4b6080ee9b47821444e99b",
)
# prettify must match the version used in Gitiles

28
java/com/google/gerrit/extensions/api/changes/FileApi.java
View File

@@ -15,10 +15,12 @@
package com.google.gerrit.extensions.api.changes;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.extensions.common.BlameInfo;
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.NotImplementedException;
import com.google.gerrit.extensions.restapi.RestApiException;
import java.util.List;
import java.util.OptionalInt;
public interface FileApi {
@@ -42,6 +44,12 @@ public interface FileApi {
/** Set the file reviewed or not reviewed */
void setReviewed(boolean reviewed) throws RestApiException;
/**
* Creates a request to retrieve the blame information. On the returned request formatting options
* for the blame request can be set.
*/
BlameRequest blameRequest() throws RestApiException;
abstract class DiffRequest {
private String base;
private Integer context;
@@ -97,6 +105,21 @@ public interface FileApi {
}
}
abstract class BlameRequest {
private boolean forBase;
public abstract List<BlameInfo> get() throws RestApiException;
public BlameRequest forBase(boolean forBase) {
this.forBase = forBase;
return this;
}
public boolean isForBase() {
return forBase;
}
}
/**
* A default implementation which allows source compatibility when adding new methods to the
* interface.
@@ -131,5 +154,10 @@ public interface FileApi {
public void setReviewed(boolean reviewed) throws RestApiException {
throw new NotImplementedException();
}
@Override
public BlameRequest blameRequest() throws RestApiException {
throw new NotImplementedException();
}
}
}

21
java/com/google/gerrit/server/api/changes/FileApiImpl.java
View File

@@ -17,16 +17,20 @@ package com.google.gerrit.server.api.changes;
import static com.google.gerrit.server.api.ApiUtil.asRestApiException;
import com.google.gerrit.extensions.api.changes.FileApi;
import com.google.gerrit.extensions.common.BlameInfo;
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.change.FileResource;
import com.google.gerrit.server.restapi.change.GetBlame;
import com.google.gerrit.server.restapi.change.GetContent;
import com.google.gerrit.server.restapi.change.GetDiff;
import com.google.gerrit.server.restapi.change.Reviewed;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import java.util.List;
class FileApiImpl implements FileApi {
interface Factory {
@@ -34,6 +38,7 @@ class FileApiImpl implements FileApi {
}
private final GetContent getContent;
private final Provider<GetBlame> getBlame;
private final GetDiff getDiff;
private final Reviewed.PutReviewed putReviewed;
private final Reviewed.DeleteReviewed deleteReviewed;
@@ -42,11 +47,13 @@ class FileApiImpl implements FileApi {
@Inject
FileApiImpl(
GetContent getContent,
Provider<GetBlame> getBlame,
GetDiff getDiff,
Reviewed.PutReviewed putReviewed,
Reviewed.DeleteReviewed deleteReviewed,
@Assisted FileResource file) {
this.getContent = getContent;
this.getBlame = getBlame;
this.getDiff = getDiff;
this.putReviewed = putReviewed;
this.deleteReviewed = deleteReviewed;
@@ -132,4 +139,18 @@ class FileApiImpl implements FileApi {
throw asRestApiException("Cannot retrieve diff", e);
}
}
@Override
public BlameRequest blameRequest() throws RestApiException {
return new BlameRequest() {
@Override
public List<BlameInfo> get() throws RestApiException {
try {
return getBlame.get().setBase(isForBase()).apply(file).value();
} catch (Exception e) {
throw asRestApiException("Cannot retrieve blame", e);
}
}
};
}
}

5
java/com/google/gerrit/server/restapi/change/GetBlame.java
View File

@@ -76,6 +76,11 @@ public class GetBlame implements RestReadView<FileResource> {
this.autoMerger = autoMerger;
}
public GetBlame setBase(boolean base) {
this.base = base;
return this;
}
@Override
public Response<List<BlameInfo>> apply(FileResource resource)
throws RestApiException, IOException, InvalidChangeOperationException {

128
javatests/com/google/gerrit/acceptance/api/revision/GetBlameIT.java Normal file
View File

@@ -0,0 +1,128 @@
// Copyright (C) 2019 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.acceptance.api.revision;
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.common.BlameInfo;
import com.google.gerrit.extensions.common.RangeInfo;
import java.util.List;
import org.junit.Test;
public class GetBlameIT extends AbstractDaemonTest {
@Test
public void forNonExistingFile() throws Exception {
PushOneCommit.Result r = createChange("Test Change", "foo.txt", "FOO");
List<BlameInfo> blameInfos =
gApi.changes().id(r.getChangeId()).current().file("non-existing.txt").blameRequest().get();
// File doesn't exist in commit.
assertThat(blameInfos).isEmpty();
}
@Test
public void forNonExistingFileFromBase() throws Exception {
PushOneCommit.Result r = createChange("Test Change", "foo.txt", "FOO");
List<BlameInfo> blameInfos =
gApi.changes()
.id(r.getChangeId())
.current()
.file("non-existing.txt")
.blameRequest()
.forBase(true)
.get();
// File doesn't exist in base commit.
assertThat(blameInfos).isEmpty();
}
@Test
public void forNewlyAddedFile() throws Exception {
PushOneCommit.Result r = createChange("Test Change", "foo.txt", "FOO");
List<BlameInfo> blameInfos =
gApi.changes().id(r.getChangeId()).current().file("foo.txt").blameRequest().get();
assertThat(blameInfos).hasSize(1);
BlameInfo blameInfo = blameInfos.get(0);
assertThat(blameInfo.author).isEqualTo(admin.fullName());
assertThat(blameInfo.id).isEqualTo(r.getCommit().getId().name());
assertThat(blameInfo.commitMsg).isEqualTo(r.getCommit().getFullMessage());
assertThat(blameInfo.time).isEqualTo(r.getCommit().getCommitTime());
assertThat(blameInfo.ranges).hasSize(1);
RangeInfo rangeInfo = blameInfo.ranges.get(0);
assertThat(rangeInfo.start).isEqualTo(1);
assertThat(rangeInfo.end).isEqualTo(1);
}
@Test
public void forNewlyAddedFileFromBase() throws Exception {
String changeId = createChange("Test Change", "foo.txt", "FOO").getChangeId();
List<BlameInfo> blameInfos =
gApi.changes().id(changeId).current().file("foo.txt").blameRequest().forBase(true).get();
// File doesn't exist in base commit.
assertThat(blameInfos).isEmpty();
}
@Test
public void forRecreatedFile() throws Exception {
// Create change that adds 'foo.txt'.
createChange("Change 1", "foo.txt", "FOO");
// Create change that deletes 'foo.txt'.
pushFactory
.create(admin.newIdent(), testRepo, "Change 2", "foo.txt", "FOO")
.rm("refs/for/master");
// Create change that recreates 'foo.txt'.
PushOneCommit.Result r = createChange("Change 3", "foo.txt", "FOO");
List<BlameInfo> blameInfos =
gApi.changes().id(r.getChangeId()).current().file("foo.txt").blameRequest().get();
assertThat(blameInfos).hasSize(1);
BlameInfo blameInfo = blameInfos.get(0);
assertThat(blameInfo.author).isEqualTo(admin.fullName());
assertThat(blameInfo.id).isEqualTo(r.getCommit().getId().name());
assertThat(blameInfo.commitMsg).isEqualTo(r.getCommit().getFullMessage());
assertThat(blameInfo.time).isEqualTo(r.getCommit().getCommitTime());
assertThat(blameInfo.ranges).hasSize(1);
RangeInfo rangeInfo = blameInfo.ranges.get(0);
assertThat(rangeInfo.start).isEqualTo(1);
assertThat(rangeInfo.end).isEqualTo(1);
}
@Test
public void forRecreatedFileFromBase() throws Exception {
// Create change that adds 'foo.txt'.
createChange("Change 1", "foo.txt", "FOO");
// Create change that deletes 'foo.txt'.
pushFactory
.create(admin.newIdent(), testRepo, "Change 2", "foo.txt", "FOO")
.rm("refs/for/master");
// Create change that recreates 'foo.txt'.
String changeId3 = createChange("Change 3", "foo.txt", "FOO").getChangeId();
List<BlameInfo> blameInfos =
gApi.changes().id(changeId3).current().file("foo.txt").blameRequest().forBase(true).get();
// File doesn't exist in base commit.
assertThat(blameInfos).isEmpty();
}
}