From f21ce2d2d4cfaf8418cfebed9c1a8a8cc5aa4330 Mon Sep 17 00:00:00 2001 From: Mark Karimi Date: Fri, 9 Dec 2022 14:10:22 +0100 Subject: [PATCH 1/3] validate check run output text and summary length --- .../github/v3/checks/CheckRunOutput.java | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java b/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java index 95a644b4..0a6de89c 100644 --- a/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java +++ b/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java @@ -7,9 +7,9 @@ * 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. @@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; +import com.google.common.base.Preconditions; import com.spotify.github.GithubStyle; import java.util.List; import java.util.Optional; @@ -87,4 +88,20 @@ public interface CheckRunOutput { * @return the optional */ Optional annotationsUrl(); + + /** + * Automatically validates the maximum length of properties. + *

+ * GitHub does not validate these properly on their side (at least in GHE 3.2) and returns 422 + * HTTP responses instead. To avoid that, let's validate the data in this client library. + */ + @Value.Check + @SuppressWarnings("checkstyle:magicnumber") + default void check() { + // max values from https://docs.github.com/en/enterprise-server@3.5/rest/checks/runs#create-a-check-run + Preconditions.checkState(summary().map(String::length).orElse(0) <= 64 * 1024, + "'summary' exceeded max length of 65535 characters"); + Preconditions.checkState(text().map(String::length).orElse(0) <= 64 * 1024, + "'text' exceeded max length of 65535 characters"); + } } From bd01797d9a26dca98ef1ea070dbe478016a16e0b Mon Sep 17 00:00:00 2001 From: Mark Karimi Date: Fri, 9 Dec 2022 14:51:59 +0100 Subject: [PATCH 2/3] Add tests for checkrun output validation --- .../github/v3/checks/CheckRunOutput.java | 4 +- .../github/v3/checks/CheckRunActionTest.java | 6 --- .../github/v3/checks/CheckRunOutputTest.java | 49 +++++++++++++++++++ 3 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java diff --git a/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java b/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java index 0a6de89c..9afbe08f 100644 --- a/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java +++ b/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java @@ -99,9 +99,9 @@ public interface CheckRunOutput { @SuppressWarnings("checkstyle:magicnumber") default void check() { // max values from https://docs.github.com/en/enterprise-server@3.5/rest/checks/runs#create-a-check-run - Preconditions.checkState(summary().map(String::length).orElse(0) <= 64 * 1024, + Preconditions.checkState(summary().map(String::length).orElse(0) < 64 * 1024, "'summary' exceeded max length of 65535 characters"); - Preconditions.checkState(text().map(String::length).orElse(0) <= 64 * 1024, + Preconditions.checkState(text().map(String::length).orElse(0) < 64 * 1024, "'text' exceeded max length of 65535 characters"); } } diff --git a/src/test/java/com/spotify/github/v3/checks/CheckRunActionTest.java b/src/test/java/com/spotify/github/v3/checks/CheckRunActionTest.java index 0646fb7c..2e7bbe0d 100644 --- a/src/test/java/com/spotify/github/v3/checks/CheckRunActionTest.java +++ b/src/test/java/com/spotify/github/v3/checks/CheckRunActionTest.java @@ -20,15 +20,9 @@ package com.spotify.github.v3.checks; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertThrows; -import com.spotify.github.FixtureHelper; -import com.spotify.github.jackson.Json; import com.spotify.github.v3.checks.ImmutableCheckRunAction.Builder; -import java.io.IOException; -import java.time.ZonedDateTime; import org.junit.Test; public class CheckRunActionTest { diff --git a/src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java b/src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java new file mode 100644 index 00000000..0247ac3a --- /dev/null +++ b/src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java @@ -0,0 +1,49 @@ +/*- + * -\-\- + * github-client + * -- + * Copyright (C) 2016 - 2022 Spotify AB + * -- + * 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.spotify.github.v3.checks; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.spotify.github.v3.checks.ImmutableCheckRunOutput.Builder; +import org.junit.Test; + +public class CheckRunOutputTest { + + private Builder builder() { + return ImmutableCheckRunOutput.builder(); + } + + @Test + public void allowsCreationWithinLimits() { + builder().build(); + builder() + .text("t".repeat((64 * 1024) - 1)).summary("s".repeat((1024 * 64) - 1)).build(); + } + + @Test + public void failsCreationWhenMaxLengthExceeded() { + assertThrows(IllegalStateException.class, + () -> builder().text("t".repeat(64 * 1024)).build()); + assertThrows(IllegalStateException.class, + () -> builder().summary("s".repeat(64 * 1024)).build()); + } +} + From 8af1820eab5bbccd94409de6dce91d2fa477e722 Mon Sep 17 00:00:00 2001 From: Mark Karimi Date: Fri, 9 Dec 2022 15:09:02 +0100 Subject: [PATCH 3/3] simplify character limits --- .../java/com/spotify/github/v3/checks/CheckRunOutput.java | 4 ++-- .../com/spotify/github/v3/checks/CheckRunOutputTest.java | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java b/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java index 9afbe08f..76014b40 100644 --- a/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java +++ b/src/main/java/com/spotify/github/v3/checks/CheckRunOutput.java @@ -99,9 +99,9 @@ public interface CheckRunOutput { @SuppressWarnings("checkstyle:magicnumber") default void check() { // max values from https://docs.github.com/en/enterprise-server@3.5/rest/checks/runs#create-a-check-run - Preconditions.checkState(summary().map(String::length).orElse(0) < 64 * 1024, + Preconditions.checkState(summary().map(String::length).orElse(0) <= 65535, "'summary' exceeded max length of 65535 characters"); - Preconditions.checkState(text().map(String::length).orElse(0) < 64 * 1024, + Preconditions.checkState(text().map(String::length).orElse(0) <= 65535, "'text' exceeded max length of 65535 characters"); } } diff --git a/src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java b/src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java index 0247ac3a..e8658c90 100644 --- a/src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java +++ b/src/test/java/com/spotify/github/v3/checks/CheckRunOutputTest.java @@ -35,15 +35,15 @@ private Builder builder() { public void allowsCreationWithinLimits() { builder().build(); builder() - .text("t".repeat((64 * 1024) - 1)).summary("s".repeat((1024 * 64) - 1)).build(); + .text("t".repeat(65535)).summary("s".repeat(65535)).build(); } @Test public void failsCreationWhenMaxLengthExceeded() { assertThrows(IllegalStateException.class, - () -> builder().text("t".repeat(64 * 1024)).build()); + () -> builder().text("t".repeat(65536)).build()); assertThrows(IllegalStateException.class, - () -> builder().summary("s".repeat(64 * 1024)).build()); + () -> builder().summary("s".repeat(65536)).build()); } }