Skip file size validation for binary files

In change I4b280bb8 we relanded the change to validate file sizes on
file diff requests. The previous behaviour for binary files prior to
I4b280bb8 used to show a message of "Difference in binary files" in the
web UI (see [1]). After adding the the file size validation, it resulted
in displaying a bad request message in the UI.

In this change, we skip validation for binary files. The git file diff
logic does not produce edits for binary files and we only set the 'skip'
parameter in DiffInfo.ContentEntry.skip to the number of lines in the
binary file. The front-end displays a "Difference in binary files"
message. The validation is not needed for binary files in this case as
not to break this existing workflow.

[1] https://gerrit.googlesource.com/gerrit/+/e261a84fb259c7e87c933155677d2311fe0e33d2/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-element.ts#365

Google-Bug-Id: b/288895561
Release-Notes: skip
Change-Id: Iefc4adf3f09032043e24ecc12ecb3560d38492c0
(cherry picked from commit ed980afc34ba88f5978dfba5fbc07db526c916fa)

2 files changed, 21 insertions, 2 deletions

diff --git a/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java b/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
index 4001ab40dd..6964d639e8 100644
--- a/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
+++ b/java/com/google/gerrit/server/patch/DiffFileSizeValidator.java
@@ -15,6 +15,7 @@
 package com.google.gerrit.server.patch;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.gerrit.entities.Patch.PatchType;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.LargeObjectException;
 import com.google.gerrit.server.patch.filediff.FileDiffOutput;
@@ -43,8 +44,14 @@ public class DiffFileSizeValidator implements DiffValidator {
 
   @Override
   public void validate(FileDiffOutput fileDiff) throws LargeObjectException {
-    if (maxFileSize <= 0) {
+    if (maxFileSize <= 0
+        || (fileDiff.patchType().isPresent()
+            && fileDiff.patchType().get().equals(PatchType.BINARY))) {
       // Do not apply limits if the config is not set.
+      // Also do not check file size for binary files. For modified binary files, JGit skips the
+      // diff and returns no edits. On the API layer, we only set the DiffInfo.ContentEntry.skip
+      // parameter to the number of lines in the file and the front-end displays a "Difference in
+      // binary files" in the diff view.
       return;
     }
     if (fileDiff.size() > maxFileSize) {
diff --git a/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java b/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
index 9cc19f5f6d..9f697fdb37 100644
--- a/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
+++ b/javatests/com/google/gerrit/server/patch/DiffValidatorsTest.java
@@ -74,13 +74,25 @@ public class DiffValidatorsTest {
     diffValidators.validate(fileDiff);
   }
 
+  @Test
+  public void binaryFileSizeExceeded_notCheckedForFileSize() throws Exception {
+    diffFileSizeValidator.setMaxFileSize(1000);
+    int largeSize = 100000000;
+    FileDiffOutput fileDiff = createFakeFileDiffOutput(largeSize, PatchType.BINARY);
+    diffValidators.validate(fileDiff);
+  }
+
   private FileDiffOutput createFakeFileDiffOutput(int largeSize) {
+    return createFakeFileDiffOutput(largeSize, PatchType.UNIFIED);
+  }
+
+  private FileDiffOutput createFakeFileDiffOutput(int largeSize, PatchType patchType) {
     return FileDiffOutput.builder()
         .oldCommitId(ObjectId.zeroId())
         .newCommitId(ObjectId.zeroId())
         .comparisonType(ComparisonType.againstRoot())
         .changeType(ChangeType.ADDED)
-        .patchType(Optional.of(PatchType.UNIFIED))
+        .patchType(Optional.of(patchType))
         .oldPath(Optional.empty())
         .newPath(Optional.of("f.txt"))
         .oldMode(Optional.empty())