Merge pull request #89 from spotify/fix-branch-protection-url-deserialize

Fix parsing non-protected branches + simplify deserialization

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();
 }
 

…/github/v3/repos/BranchDeserializer.java …pos/BranchProtectionUrlDeserializer.javasrc/main/java/com/spotify/github/v3/repos/BranchDeserializer.java renamed to src/main/java/com/spotify/github/v3/repos/BranchProtectionUrlDeserializer.java 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
 }