Fix parsing non-protected branches + simplify deserialization

In #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.

Author: dziemba

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<Boolean> isProtected();
 
   /** Branch protection API URL */
+  @JsonDeserialize(using = BranchProtectionUrlDeserializer.class)
   Optional<URI> protectionUrl();
 }
 

src/main/java/com/spotify/github/v3/repos/BranchDeserializer.java renamed to 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<ImmutableBranch> {
+public class BranchProtectionUrlDeserializer extends JsonDeserializer<Optional<URI>> {
 
-  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<URI> 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<Optional<String>> ref = new TypeReference<>() {};
+    Optional<String> protectionUrlStringOpt = jsonParser.readValueAs(ref);
+
+    return protectionUrlStringOpt.map(
+        protectionUrlString -> {
+          try {
+            return new URI(protectionUrlString);
+          } catch (URISyntaxException e) {
+            return fixInvalidGithubUrl(protectionUrlString);
+          }
+        });
   }
 }

src/test/java/com/spotify/github/v3/clients/RepositoryClientTest.java

@@ -251,21 +251,34 @@ 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(),
         is("https://api.github.com/repos/octocat/Hello-World/commits/c5b97d5ae6c19d5c5df71a34c7fbeeda2479ccbc"));
   }
 
   @Test
-  public void getBranchWithoutProtection() throws Exception {
-    // Make sure the custom deserialiser correctly handles the optional protected fields
+  public void getBranchWithNoProtection() throws Exception {
     final CompletableFuture<Branch> 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<Branch> 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(),

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"
+  }
+}

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
 }