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";