Remove fallback to check entire files
Now that we have figured out object visibility in Gerrit core, we
can add a test to check the diff instead of the whole file even
when changes/patch sets are created on the REST API.
Depends-On: 306930
Change-Id: I6e1d99f3fbf411a97ec0641e8633886812909864
diff --git a/src/main/java/com/googlesource/gerrit/plugins/uploadvalidator/BlockedKeywordValidator.java b/src/main/java/com/googlesource/gerrit/plugins/uploadvalidator/BlockedKeywordValidator.java
index 64ce6ab..1e805e9 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/uploadvalidator/BlockedKeywordValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/uploadvalidator/BlockedKeywordValidator.java
@@ -179,7 +179,10 @@
}
}
}
- } catch (NoSuchProjectException | IOException | ExecutionException e) {
+ } catch (NoSuchProjectException
+ | IOException
+ | ExecutionException
+ | PatchListNotAvailableException e) {
throw new CommitValidationException("failed to check on blocked keywords", e);
}
return Collections.emptyList();
@@ -221,24 +224,14 @@
RevWalk revWalk,
ImmutableCollection<Pattern> blockedKeywordPatterns,
PluginConfig cfg)
- throws IOException, ExecutionException {
+ throws IOException, ExecutionException, PatchListNotAvailableException {
List<CommitValidationMessage> messages = new LinkedList<>();
checkCommitMessageForBlockedKeywords(blockedKeywordPatterns, messages, c.getFullMessage());
Map<String, ObjectId> content = CommitUtils.getChangedContent(repo, c, revWalk);
- Optional<PatchList> patchList;
- try {
- patchList =
- Optional.of(
- patchListCache.get(
- PatchListKey.againstDefaultBase(c, DiffPreferencesInfo.Whitespace.IGNORE_NONE),
- project));
- } catch (PatchListNotAvailableException e) {
- // TODO(hiesel): Make changes to Gerrit core to always get a diff here. This entails figuring
- // out Git object visibility between unflushed ObjectInserters and ObjectReaders created from
- // the repo.
- logger.atInfo().withCause(e).log("unable to load patch set");
- patchList = Optional.empty();
- }
+ PatchList patchList =
+ patchListCache.get(
+ PatchListKey.againstDefaultBase(c, DiffPreferencesInfo.Whitespace.IGNORE_NONE),
+ project);
for (String path : content.keySet()) {
ObjectLoader ol = revWalk.getObjectReader().open(content.get(path));
@@ -247,12 +240,8 @@
continue;
}
}
- if (patchList.isPresent()) {
- checkLineDiffForBlockedKeywords(
- patchList.get().get(path).getEdits(), blockedKeywordPatterns, messages, path, ol);
- } else {
- checkEntireFileForBlockedKeywords(blockedKeywordPatterns, messages, path, ol);
- }
+ checkLineDiffForBlockedKeywords(
+ patchList.get(path).getEdits(), blockedKeywordPatterns, messages, path, ol);
}
return messages;
}
@@ -304,22 +293,6 @@
}
}
- private static void checkEntireFileForBlockedKeywords(
- ImmutableCollection<Pattern> blockedKeywordPatterns,
- List<CommitValidationMessage> messages,
- String path,
- ObjectLoader ol)
- throws IOException {
- try (BufferedReader br =
- new BufferedReader(new InputStreamReader(ol.openStream(), StandardCharsets.UTF_8))) {
- int line = 1;
- for (String l = br.readLine(); l != null; l = br.readLine()) {
- checkLineForBlockedKeywords(blockedKeywordPatterns, messages, path, line, l);
- line++;
- }
- }
- }
-
private static List<String> findBlockedKeywordsInString(
ImmutableCollection<Pattern> blockedKeywordPatterns, String text) {
List<String> found = new ArrayList<>();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/uploadvalidator/UploadValidatorIT.java b/src/test/java/com/googlesource/gerrit/plugins/uploadvalidator/UploadValidatorIT.java
index bd5f8f3..0e9bf85 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/uploadvalidator/UploadValidatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/uploadvalidator/UploadValidatorIT.java
@@ -386,9 +386,31 @@
.create(admin.newIdent(), clone, "Subject", "foo.txt", "" + "blah \n")
.to("refs/heads/stable")
.assertOkStatus();
- // Create a change that merges the other branch into master. This defaults back to
- // full-file validation. If it doesn't, the create change call below would fail with
- // a MissingObjectException.
+ ChangeInput changeInput = new ChangeInput();
+ changeInput.project = project.get();
+ changeInput.branch = "master";
+ changeInput.subject = "A change";
+ changeInput.status = ChangeStatus.NEW;
+ MergeInput mergeInput = new MergeInput();
+ mergeInput.source = gApi.projects().name(project.get()).branch("stable").get().revision;
+ changeInput.merge = mergeInput;
+ gApi.changes().create(changeInput);
+ }
+
+ @Test
+ public void createChangeSucceedsWhenKeywordExistsInFileButNotInDiff() throws Exception {
+ RevCommit head = getHead(testRepo.getRepository(), "HEAD");
+ pushFactory
+ .create(admin.newIdent(), clone, "Subject", "file.txt", "" + "secr3t \n")
+ .to("refs/heads/master")
+ .assertOkStatus();
+ pushConfig(
+ Joiner.on("\n").join("[plugin \"uploadvalidator\"]", " blockedKeywordPattern = secr3t"));
+ clone.reset(head);
+ pushFactory
+ .create(admin.newIdent(), clone, "Subject", "foo.txt", "" + "blah \n")
+ .to("refs/heads/stable")
+ .assertOkStatus();
ChangeInput changeInput = new ChangeInput();
changeInput.project = project.get();
changeInput.branch = "master";
@@ -414,9 +436,6 @@
.create(admin.newIdent(), clone, "Subject", "foo.txt", "" + "blah \n")
.to("refs/heads/stable")
.assertOkStatus();
- // Create a change that merges the other branch into master. This defaults back to
- // full-file validation. If it doesn't, the create change call below would fail with
- // a MissingObjectException.
ChangeInput changeInput = new ChangeInput();
changeInput.project = project.get();
changeInput.branch = "master";
