From 547597f39d6b0d04bdd91f82bc61739c21373868 Mon Sep 17 00:00:00 2001 From: Niko Dziemba Date: Wed, 13 Apr 2022 12:46:33 +0200 Subject: [PATCH] Fix parsing non-protected branches + simplify deserialization In https://github.com/spotify/github-java-client/pull/82 we added a custom deserializer for the `Branch` class. This deserializer didn't handle certain cases properly that the GH API would return. In particular the case where the `protected` field was set and no `protection_url` is present would lead to a NPE in the deserializer. To avoid this error and make the whole parser more resilient to different API results, let's reduce the scope of the custom deserializer to only parse the `protection_url` field instead of the whole `Branch` class. There should be no change in runtime behavior apart from fixing the error cases. --- .../com/spotify/github/v3/repos/Branch.java | 3 +- ...a => BranchProtectionUrlDeserializer.java} | 47 ++++++++----------- .../v3/clients/RepositoryClientTest.java | 17 ++++++- .../v3/repos/branch-no-protection-fields.json | 7 +++ .../github/v3/repos/branch-not-protected.json | 3 +- 5 files changed, 45 insertions(+), 32 deletions(-) rename src/main/java/com/spotify/github/v3/repos/{BranchDeserializer.java => BranchProtectionUrlDeserializer.java} (57%) create mode 100644 src/test/resources/com/spotify/github/v3/repos/branch-no-protection-fields.json diff --git a/src/main/java/com/spotify/github/v3/repos/Branch.java b/src/main/java/com/spotify/github/v3/repos/Branch.java index f3589142..28701592 100644 --- a/src/main/java/com/spotify/github/v3/repos/Branch.java +++ b/src/main/java/com/spotify/github/v3/repos/Branch.java @@ -34,7 +34,7 @@ @Value.Immutable @GithubStyle @JsonSerialize(as = ImmutableBranch.class) -@JsonDeserialize(as = ImmutableBranch.class, using = BranchDeserializer.class) +@JsonDeserialize(as = ImmutableBranch.class) public interface Branch { /** Branch name */ @@ -50,6 +50,7 @@ public interface Branch { Optional isProtected(); /** Branch protection API URL */ + @JsonDeserialize(using = BranchProtectionUrlDeserializer.class) Optional protectionUrl(); } diff --git a/src/main/java/com/spotify/github/v3/repos/BranchDeserializer.java b/src/main/java/com/spotify/github/v3/repos/BranchProtectionUrlDeserializer.java similarity index 57% rename from src/main/java/com/spotify/github/v3/repos/BranchDeserializer.java rename to src/main/java/com/spotify/github/v3/repos/BranchProtectionUrlDeserializer.java index c67b8ff0..35f766ca 100644 --- a/src/main/java/com/spotify/github/v3/repos/BranchDeserializer.java +++ b/src/main/java/com/spotify/github/v3/repos/BranchProtectionUrlDeserializer.java @@ -2,7 +2,7 @@ * -\-\- * github-api * -- - * Copyright (C) 2016 - 2020 Spotify AB + * Copyright (C) 2016 - 2021 Spotify AB * -- * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,19 +21,19 @@ package com.spotify.github.v3.repos; import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JsonDeserializer; -import com.fasterxml.jackson.databind.JsonNode; -import com.spotify.github.v3.git.ImmutableShaLink; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.util.Optional; -public class BranchDeserializer extends JsonDeserializer { +public class BranchProtectionUrlDeserializer extends JsonDeserializer> { - private URI fixInvalidGithubUrl(final String invalidUrl) throws IOException { + private URI fixInvalidGithubUrl(final String invalidUrl) { // There's a bug in github where it gives you back non-url-encoded characters // in the protection_url field. For example if your branch has a single "%" in its name. // As of this writing, the protection URL looks something like this @@ -48,29 +48,20 @@ private URI fixInvalidGithubUrl(final String invalidUrl) throws IOException { } @Override - public ImmutableBranch deserialize(final JsonParser jsonParser, - final DeserializationContext deserializationContext) + public Optional deserialize( + final JsonParser jsonParser, final DeserializationContext deserializationContext) throws IOException { - JsonNode node = jsonParser.getCodec().readTree(jsonParser); - URI protectionUrl; - var immutableBranchBuilder = ImmutableBranch.builder(); - if (node.has("protected")) { - immutableBranchBuilder.isProtected(node.get("protected").asBoolean()); - String protectionUrlString = node.get("protection_url").asText(); - try { - protectionUrl = new URI(protectionUrlString); - } catch (URISyntaxException e) { - protectionUrl = fixInvalidGithubUrl(protectionUrlString); - } - immutableBranchBuilder.protectionUrl(protectionUrl); - } - ImmutableShaLink shaLink = ImmutableShaLink.builder() - .sha(node.get("commit").get("sha").asText()) - .url(URI.create(node.at("/commit/url").asText())) - .build(); - return immutableBranchBuilder - .name(node.get("name").textValue()) - .commit(shaLink) - .build(); + + TypeReference> ref = new TypeReference<>() {}; + Optional protectionUrlStringOpt = jsonParser.readValueAs(ref); + + return protectionUrlStringOpt.map( + protectionUrlString -> { + try { + return new URI(protectionUrlString); + } catch (URISyntaxException e) { + return fixInvalidGithubUrl(protectionUrlString); + } + }); } } diff --git a/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java b/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java index dd737532..d92a8169 100644 --- a/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java +++ b/src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java @@ -251,6 +251,7 @@ public void getBranch() throws Exception { .thenReturn(fixture); final Branch branch = repoClient.getBranch("somebranch").get(); assertThat(branch.isProtected().orElse(false), is(true)); + assertThat(branch.protectionUrl().get().toString(), is("https://api.github.com/repos/octocat/Hello-World/branches/master/protection")); assertThat(branch.commit().sha(), is("6dcb09b5b57875f334f61aebed695e2e4193db5e")); assertThat( branch.commit().url().toString(), @@ -258,14 +259,26 @@ public void getBranch() throws Exception { } @Test - public void getBranchWithoutProtection() throws Exception { - // Make sure the custom deserialiser correctly handles the optional protected fields + public void getBranchWithNoProtection() throws Exception { final CompletableFuture fixture = completedFuture(json.fromJson(getFixture("branch-not-protected.json"), Branch.class)); when(github.request("/repos/someowner/somerepo/branches/somebranch", Branch.class)) .thenReturn(fixture); final Branch branch = repoClient.getBranch("somebranch").get(); assertThat(branch.isProtected().orElse(false), is(false)); + assertTrue(branch.protectionUrl().isEmpty()); + assertThat(branch.commit().sha(), is("6dcb09b5b57875f334f61aebed695e2e4193db5e")); + } + + @Test + public void getBranchWithoutProtectionFields() throws Exception { + final CompletableFuture fixture = + completedFuture(json.fromJson(getFixture("branch-no-protection-fields.json"), Branch.class)); + when(github.request("/repos/someowner/somerepo/branches/somebranch", Branch.class)) + .thenReturn(fixture); + final Branch branch = repoClient.getBranch("somebranch").get(); + assertThat(branch.isProtected().orElse(false), is(false)); + assertTrue(branch.protectionUrl().isEmpty()); assertThat(branch.commit().sha(), is("6dcb09b5b57875f334f61aebed695e2e4193db5e")); assertThat( branch.commit().url().toString(), diff --git a/src/test/resources/com/spotify/github/v3/repos/branch-no-protection-fields.json b/src/test/resources/com/spotify/github/v3/repos/branch-no-protection-fields.json new file mode 100644 index 00000000..023b3d02 --- /dev/null +++ b/src/test/resources/com/spotify/github/v3/repos/branch-no-protection-fields.json @@ -0,0 +1,7 @@ +{ + "name": "master", + "commit": { + "sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e", + "url": "https://api.github.com/repos/octocat/Hello-World/commits/c5b97d5ae6c19d5c5df71a34c7fbeeda2479ccbc" + } +} diff --git a/src/test/resources/com/spotify/github/v3/repos/branch-not-protected.json b/src/test/resources/com/spotify/github/v3/repos/branch-not-protected.json index 023b3d02..ed5ad96f 100644 --- a/src/test/resources/com/spotify/github/v3/repos/branch-not-protected.json +++ b/src/test/resources/com/spotify/github/v3/repos/branch-not-protected.json @@ -3,5 +3,6 @@ "commit": { "sha": "6dcb09b5b57875f334f61aebed695e2e4193db5e", "url": "https://api.github.com/repos/octocat/Hello-World/commits/c5b97d5ae6c19d5c5df71a34c7fbeeda2479ccbc" - } + }, + "protected": false }