Merge branch 'malowe/main/131' into 'main'

Iss #131 - Add logic to lookup user by Github ID when available

See merge request !138

pom.xml

@@ -5,7 +5,7 @@
   <artifactId>git-eca</artifactId>
   <version>1.1.0</version>
   <properties>
-    <eclipse-api-version>0.7.3</eclipse-api-version>
+    <eclipse-api-version>0.7.4</eclipse-api-version>
     <compiler-plugin.version>3.8.1</compiler-plugin.version>
     <maven.compiler.parameters>true</maven.compiler.parameters>
     <maven.compiler.source>11</maven.compiler.source>

src/main/java/org/eclipsefoundation/git/eca/api/models/EclipseUser.java

@@ -14,6 +14,7 @@ package org.eclipsefoundation.git.eca.api.models;
 import javax.annotation.Nullable;
 
 import org.eclipsefoundation.git.eca.model.GitUser;
+
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
@@ -37,6 +38,9 @@ public abstract class EclipseUser {
 
     public abstract boolean getIsCommitter();
 
+    @Nullable
+    public abstract String getGithubHandle();
+
     @Nullable
     @JsonIgnore
     public abstract Boolean getIsBot();
@@ -75,6 +79,8 @@ public abstract class EclipseUser {
 
         public abstract Builder setIsCommitter(boolean isCommitter);
 
+        public abstract Builder setGithubHandle(@Nullable String githubHandle);
+
         @JsonIgnore
         public abstract Builder setIsBot(@Nullable Boolean isBot);
 

src/main/java/org/eclipsefoundation/git/eca/api/models/GithubCommit.java

@@ -11,6 +11,8 @@
  */
 package org.eclipsefoundation.git.eca.api.models;
 
+import java.util.List;
+
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
 import com.google.auto.value.AutoValue;
@@ -23,8 +25,15 @@ import com.google.auto.value.AutoValue;
 @JsonDeserialize(builder = AutoValue_GithubCommit.Builder.class)
 public abstract class GithubCommit {
     public abstract String getSha();
+
     public abstract CommitData getCommit();
 
+    public abstract CommitUser getCommitter();
+
+    public abstract CommitUser getAuthor();
+
+    public abstract List<ParentCommit> getParents();
+
     public static Builder builder() {
         return new AutoValue_GithubCommit.Builder();
     }
@@ -33,16 +42,24 @@ public abstract class GithubCommit {
     @JsonPOJOBuilder(withPrefix = "set")
     public abstract static class Builder {
         public abstract Builder setSha(String sha);
+
         public abstract Builder setCommit(CommitData commit);
 
+        public abstract Builder setCommitter(CommitUser committer);
+
+        public abstract Builder setAuthor(CommitUser author);
+
+        public abstract Builder setParents(List<ParentCommit> parents);
+
         public abstract GithubCommit build();
     }
 
     @AutoValue
     @JsonDeserialize(builder = AutoValue_GithubCommit_CommitData.Builder.class)
     public abstract static class CommitData {
-        public abstract CommitUser getAuthor();
-        public abstract CommitUser getCommitter();
+        public abstract BriefCommitUser getAuthor();
+
+        public abstract BriefCommitUser getCommitter();
 
         public static Builder builder() {
             return new AutoValue_GithubCommit_CommitData.Builder();
@@ -51,29 +68,69 @@ public abstract class GithubCommit {
         @AutoValue.Builder
         @JsonPOJOBuilder(withPrefix = "set")
         public abstract static class Builder {
-            public abstract Builder setAuthor(CommitUser author);
-            public abstract Builder setCommitter(CommitUser committer);
+            public abstract Builder setAuthor(BriefCommitUser author);
+
+            public abstract Builder setCommitter(BriefCommitUser committer);
 
             public abstract CommitData build();
         }
     }
+
     @AutoValue
-    @JsonDeserialize(builder = AutoValue_GithubCommit_CommitUser.Builder.class)
-    public abstract static class CommitUser {
+    @JsonDeserialize(builder = AutoValue_GithubCommit_BriefCommitUser.Builder.class)
+    public abstract static class BriefCommitUser {
         public abstract String getName();
+
         public abstract String getEmail();
 
         public static Builder builder() {
-            return new AutoValue_GithubCommit_CommitUser.Builder();
+            return new AutoValue_GithubCommit_BriefCommitUser.Builder();
         }
 
         @AutoValue.Builder
         @JsonPOJOBuilder(withPrefix = "set")
         public abstract static class Builder {
             public abstract Builder setName(String name);
+
             public abstract Builder setEmail(String email);
 
+            public abstract BriefCommitUser build();
+        }
+    }
+
+    @AutoValue
+    @JsonDeserialize(builder = AutoValue_GithubCommit_CommitUser.Builder.class)
+    public abstract static class CommitUser {
+        public abstract String getLogin();
+
+        public static Builder builder() {
+            return new AutoValue_GithubCommit_CommitUser.Builder();
+        }
+
+        @AutoValue.Builder
+        @JsonPOJOBuilder(withPrefix = "set")
+        public abstract static class Builder {
+            public abstract Builder setLogin(String login);
+
             public abstract CommitUser build();
         }
     }
+
+    @AutoValue
+    @JsonDeserialize(builder = AutoValue_GithubCommit_ParentCommit.Builder.class)
+    public abstract static class ParentCommit {
+        public abstract String getSha();
+
+        public static Builder builder() {
+            return new AutoValue_GithubCommit_ParentCommit.Builder();
+        }
+
+        @AutoValue.Builder
+        @JsonPOJOBuilder(withPrefix = "set")
+        public abstract static class Builder {
+            public abstract Builder setSha(String sha);
+
+            public abstract ParentCommit build();
+        }
+    }
 }

src/main/java/org/eclipsefoundation/git/eca/model/GitUser.java

@@ -32,6 +32,9 @@ public abstract class GitUser {
     @Nullable
     public abstract String getMail();
 
+    @Nullable
+    public abstract String getExternalId();
+
     public static Builder builder() {
         return new AutoValue_GitUser.Builder();
     }
@@ -43,6 +46,8 @@ public abstract class GitUser {
 
         public abstract Builder setMail(@Nullable String mail);
 
+        public abstract Builder setExternalId(@Nullable String externalId);
+
         public abstract GitUser build();
     }
 }
\ No newline at end of file

src/main/java/org/eclipsefoundation/git/eca/resource/GithubAdjacentResource.java

@@ -30,6 +30,7 @@ import org.eclipsefoundation.core.model.RequestWrapper;
 import org.eclipsefoundation.core.service.APIMiddleware;
 import org.eclipsefoundation.git.eca.api.GithubAPI;
 import org.eclipsefoundation.git.eca.api.models.GithubCommit;
+import org.eclipsefoundation.git.eca.api.models.GithubCommit.ParentCommit;
 import org.eclipsefoundation.git.eca.api.models.GithubCommitStatusRequest;
 import org.eclipsefoundation.git.eca.api.models.GithubWebhookRequest;
 import org.eclipsefoundation.git.eca.api.models.GithubWebhookRequest.PullRequest;
@@ -122,12 +123,15 @@ public abstract class GithubAdjacentResource {
                                         .builder()
                                         .setMail(c.getCommit().getAuthor().getEmail())
                                         .setName(c.getCommit().getAuthor().getName())
+                                        .setExternalId(c.getAuthor().getLogin())
                                         .build())
                                 .setCommitter(GitUser
                                         .builder()
                                         .setMail(c.getCommit().getCommitter().getEmail())
                                         .setName(c.getCommit().getCommitter().getName())
+                                        .setExternalId(c.getCommitter().getLogin())
                                         .build())
+                                .setParents(c.getParents().stream().map(ParentCommit::getSha).collect(Collectors.toList()))
                                 .build())
                         .collect(Collectors.toList()))
                 .build();

src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationService.java

@@ -428,6 +428,20 @@ public class DefaultValidationService implements ValidationService {
      * @return the Eclipse Account user information if found, or null if there was an error or no user exists.
      */
     private EclipseUser getIdentifiedUser(GitUser user) {
+        // check if the external ID is set, and if so, attempt to look the user up.
+        if (StringUtils.isNotBlank(user.getExternalId())) {
+            // right now this is only supported for Github account lookups, so that will be used
+            EclipseUser actualUser = users.getUserByGithubUsername(user.getExternalId());
+            // if present, return the user. Otherwise, log and continue processing
+            if (actualUser != null) {
+                return actualUser;
+            } else {
+                LOGGER
+                        .debug("An external ID of {} was passed, but no matching Eclipse users found. Falling back on default email lookup.",
+                                user.getExternalId());
+            }
+        }
+
         // don't process an empty email as it will never have a match
         if (StringUtils.isBlank(user.getMail())) {
             LOGGER.debug("Cannot get identified user if user is empty, returning null");

src/test/java/org/eclipsefoundation/git/eca/resource/ValidationResourceTest.java

@@ -552,13 +552,33 @@ class ValidationResourceTest {
                 .testPost(VALIDATE_FORBIDDEN_CASE, createGerritRequest(true, "/gitroot/sample/gerrit.other-project", Arrays.asList(c1)));
     }
 
+    @Test
+    void validate_githubExternalId_success() {
+        GitUser g1 = GitUser.builder().setName("grunter").setMail("other-grunt@test.co").setExternalId("grunter2").build();
+        Commit c1 = createNoBodyCommit(g1, g1);
+
+        RestAssuredTemplates
+                .testPost(VALIDATE_SUCCESS_CASE,
+                        createGitHubRequest(false, "http://www.github.com/eclipsefdn/prototype", Arrays.asList(c1)));
+    }
+
+    @Test
+    void validate_githubExternalId_success_fallback() {
+        GitUser g1 = GitUser.builder().setName("grunter").setMail("grunt@important.co").setExternalId("grunter").build();
+        Commit c1 = createNoBodyCommit(g1, g1);
+
+        RestAssuredTemplates
+                .testPost(VALIDATE_SUCCESS_CASE,
+                        createGitHubRequest(false, "http://www.github.com/eclipsefdn/prototype", Arrays.asList(c1)));
+    }
+
     /*
      * NO REPLY TESTS
      */
 
     @Test
     void validateGithubNoReply_legacy() {
-        GitUser g1 = GitUser.builder().setName("grunter").setMail("grunter@users.noreply.github.com").build();
+        GitUser g1 = GitUser.builder().setName("grunter").setMail("grunter2@users.noreply.github.com").build();
 
         Commit c1 = createNoBodyCommit(g1, g1);
 
@@ -570,7 +590,7 @@ class ValidationResourceTest {
     @Test
     void validateGithubNoReply_success() {
         // sometimes the user ID and user name are reversed
-        GitUser g1 = GitUser.builder().setName("grunter").setMail("123456789+grunter@users.noreply.github.com").build();
+        GitUser g1 = GitUser.builder().setName("grunter").setMail("123456789+grunter2@users.noreply.github.com").build();
 
         Commit c1 = createNoBodyCommit(g1, g1);
 
@@ -607,7 +627,7 @@ class ValidationResourceTest {
 
     @Test
     void validateAllowListAuthor_success() {
-        GitUser g1 = GitUser.builder().setName("grunter").setMail("grunter@users.noreply.github.com").build();
+        GitUser g1 = GitUser.builder().setName("grunter").setMail("grunter2@users.noreply.github.com").build();
         GitUser g2 = GitUser.builder().setName("grunter").setMail("noreply@github.com").build();
 
         Commit c1 = createNoBodyCommit(g2, g1);
@@ -619,7 +639,7 @@ class ValidationResourceTest {
 
     @Test
     void validateAllowListCommitter_success() {
-        GitUser g1 = GitUser.builder().setName("grunter").setMail("grunter@users.noreply.github.com").build();
+        GitUser g1 = GitUser.builder().setName("grunter").setMail("grunter2@users.noreply.github.com").build();
         GitUser g2 = GitUser.builder().setName("grunter").setMail("noreply@github.com").build();
 
         Commit c1 = createNoBodyCommit(g1, g2);

src/test/java/org/eclipsefoundation/git/eca/service/impl/CachedUserServiceTest.java

@@ -52,7 +52,7 @@ class CachedUserServiceTest {
 
     @Test
     void getUser_noReplyGH_success() {
-        EclipseUser u = users.getUser("123456789+grunter@users.noreply.github.com");
+        EclipseUser u = users.getUser("123456789+grunter2@users.noreply.github.com");
         // assert that this is the user we expect and that it exists
         Assertions.assertNotNull(u);
         Assertions.assertEquals("grunter", u.getName());
@@ -76,7 +76,7 @@ class CachedUserServiceTest {
 
     @Test
     void getUserByGithubUsername_success() {
-        EclipseUser u = users.getUserByGithubUsername("grunter");
+        EclipseUser u = users.getUserByGithubUsername("grunter2");
         // assert that this is the user we expect and that it exists
         Assertions.assertNotNull(u);
         Assertions.assertEquals("grunt@important.co", u.getMail());

src/test/java/org/eclipsefoundation/git/eca/test/api/MockAccountsAPI.java

@@ -26,8 +26,8 @@ import org.eclipsefoundation.git.eca.api.models.EclipseUser.ECA;
 import io.quarkus.test.Mock;
 
 /**
- * Simple stub for accounts API. Allows for easy testing of users that don't
- * really exist upstream, and so that we don't need a real auth token for data.
+ * Simple stub for accounts API. Allows for easy testing of users that don't really exist upstream, and so that we don't
+ * need a real auth token for data.
  * 
  * @author Martin Lowe
  *
@@ -42,42 +42,68 @@ public class MockAccountsAPI implements AccountsAPI {
     public MockAccountsAPI() {
         int id = 0;
         this.users = new HashMap<>();
-        users.put("newbie@important.co", EclipseUser.builder().setIsCommitter(false).setUid(id++)
-                .setMail("newbie@important.co").setName("newbieAnon").setECA(ECA.builder().build())
-                .build());
-        users.put("slom@eclipse-foundation.org",
-                EclipseUser.builder().setIsCommitter(false).setUid(id++)
-                        .setMail("slom@eclipse-foundation.org")
-                        .setName("barshall_blathers")
-                        .setECA(ECA.builder().setCanContributeSpecProject(true).setSigned(true)
-                                .build())
-                        .build());
-        users.put("tester@eclipse-foundation.org",
-                EclipseUser.builder().setIsCommitter(false).setUid(id++)
-                        .setMail("tester@eclipse-foundation.org")
-                        .setName("mctesterson")
-                        .setECA(ECA.builder().setCanContributeSpecProject(false).setSigned(true)
-                                .build())
-                        .build());
-        users.put("code.wiz@important.co",
-                EclipseUser.builder().setIsCommitter(true).setUid(id++).setMail("code.wiz@important.co")
-                        .setName("da_wizz")
-                        .setECA(ECA.builder().setCanContributeSpecProject(true).setSigned(true)
-                                .build())
-                        .build());
-        users.put("grunt@important.co",
-                EclipseUser.builder().setIsCommitter(true).setUid(id++).setMail("grunt@important.co")
-                        .setName("grunter")
-                        .setECA(ECA.builder().setCanContributeSpecProject(false).setSigned(true)
-                                .build())
-                        .build());
-        users.put("paper.pusher@important.co",
-                EclipseUser.builder().setIsCommitter(false).setUid(id++)
-                        .setMail("paper.pusher@important.co")
-                        .setName("sumAnalyst")
-                        .setECA(ECA.builder().setCanContributeSpecProject(false).setSigned(true)
-                                .build())
-                        .build());
+        users
+                .put("newbie@important.co",
+                        EclipseUser
+                                .builder()
+                                .setIsCommitter(false)
+                                .setUid(id++)
+                                .setMail("newbie@important.co")
+                                .setName("newbieAnon")
+                                .setECA(ECA.builder().build())
+                                .build());
+        users
+                .put("slom@eclipse-foundation.org",
+                        EclipseUser
+                                .builder()
+                                .setIsCommitter(false)
+                                .setUid(id++)
+                                .setMail("slom@eclipse-foundation.org")
+                                .setName("barshall_blathers")
+                                .setECA(ECA.builder().setCanContributeSpecProject(true).setSigned(true).build())
+                                .build());
+        users
+                .put("tester@eclipse-foundation.org",
+                        EclipseUser
+                                .builder()
+                                .setIsCommitter(false)
+                                .setUid(id++)
+                                .setMail("tester@eclipse-foundation.org")
+                                .setName("mctesterson")
+                                .setECA(ECA.builder().setCanContributeSpecProject(false).setSigned(true).build())
+                                .build());
+        users
+                .put("code.wiz@important.co",
+                        EclipseUser
+                                .builder()
+                                .setIsCommitter(true)
+                                .setUid(id++)
+                                .setMail("code.wiz@important.co")
+                                .setName("da_wizz")
+                                .setECA(ECA.builder().setCanContributeSpecProject(true).setSigned(true).build())
+                                .setGithubHandle("wiz_in_da_hub")
+                                .build());
+        users
+                .put("grunt@important.co",
+                        EclipseUser
+                                .builder()
+                                .setIsCommitter(true)
+                                .setUid(id++)
+                                .setMail("grunt@important.co")
+                                .setName("grunter")
+                                .setECA(ECA.builder().setCanContributeSpecProject(false).setSigned(true).build())
+                                .setGithubHandle("grunter2")
+                                .build());
+        users
+                .put("paper.pusher@important.co",
+                        EclipseUser
+                                .builder()
+                                .setIsCommitter(false)
+                                .setUid(id++)
+                                .setMail("paper.pusher@important.co")
+                                .setName("sumAnalyst")
+                                .setECA(ECA.builder().setCanContributeSpecProject(false).setSigned(true).build())
+                                .build());
 
     }
 
@@ -89,7 +115,12 @@ public class MockAccountsAPI implements AccountsAPI {
     @Override
     public EclipseUser getUserByGithubUname(String token, String uname) {
         // assumes github id is same as uname for purposes of lookup (simplifies fetch logic)
-        return users.values().stream().filter(u -> u.getName().equals(uname)).findFirst().orElseGet(() -> null);
+        return users
+                .values()
+                .stream()
+                .filter(u -> u.getGithubHandle() != null && u.getGithubHandle().equals(uname))
+                .findFirst()
+                .orElseGet(() -> null);
     }
 
 }