From 873bae72f7382589336a5c5b59bb82e1cd9cc317 Mon Sep 17 00:00:00 2001
From: Martin Lowe <martin.lowe@eclipse-foundation.org>
Date: Wed, 31 May 2023 16:23:04 -0400
Subject: [PATCH 1/2] Iss #131 - Add logic to lookup user by Github ID when
available
An issue exists where there are inconsistencies with the email used for
commits and the associated Github user account. To patch this, a change
was made to optionally include the Github username in the validation
request. This will be used first when doing lookups of the user, falling
back on the default flow if not set.
---
pom.xml | 2 +-
.../git/eca/api/models/GithubCommit.java | 71 +++++++++++++++++--
.../git/eca/model/GitUser.java | 5 ++
.../eca/resource/GithubAdjacentResource.java | 4 ++
.../impl/DefaultValidationService.java | 14 ++++
5 files changed, 88 insertions(+), 8 deletions(-)
diff --git a/pom.xml b/pom.xml
index 98fd510e..2757a2b0 100644
--- a/pom.xml
+++ b/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>
diff --git a/src/main/java/org/eclipsefoundation/git/eca/api/models/GithubCommit.java b/src/main/java/org/eclipsefoundation/git/eca/api/models/GithubCommit.java
index 811c9d14..3cb2ea3e 100644
--- a/src/main/java/org/eclipsefoundation/git/eca/api/models/GithubCommit.java
+++ b/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();
+ }
+ }
}
diff --git a/src/main/java/org/eclipsefoundation/git/eca/model/GitUser.java b/src/main/java/org/eclipsefoundation/git/eca/model/GitUser.java
index 0ba6d5e5..03ac8457 100644
--- a/src/main/java/org/eclipsefoundation/git/eca/model/GitUser.java
+++ b/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
diff --git a/src/main/java/org/eclipsefoundation/git/eca/resource/GithubAdjacentResource.java b/src/main/java/org/eclipsefoundation/git/eca/resource/GithubAdjacentResource.java
index e048f31a..5fc19e28 100644
--- a/src/main/java/org/eclipsefoundation/git/eca/resource/GithubAdjacentResource.java
+++ b/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();
diff --git a/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationService.java b/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationService.java
index 9d98fc50..647252e0 100644
--- a/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationService.java
+++ b/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");
--
GitLab
From 2f2a1c278eb2f03723d4fe3473e12569e0b0d218 Mon Sep 17 00:00:00 2001
From: Martin Lowe <martin.lowe@eclipse-foundation.org>
Date: Thu, 1 Jun 2023 14:59:09 -0400
Subject: [PATCH 2/2] Add tests that allow for checking external ID field works
as intended
Tests added include using the wrong email but right handle and the
reverse, as well as updating the mock data to handle the new field
properly.
---
.../git/eca/api/models/EclipseUser.java | 6 +
.../eca/resource/ValidationResourceTest.java | 28 ++++-
.../service/impl/CachedUserServiceTest.java | 4 +-
.../git/eca/test/api/MockAccountsAPI.java | 109 +++++++++++-------
4 files changed, 102 insertions(+), 45 deletions(-)
diff --git a/src/main/java/org/eclipsefoundation/git/eca/api/models/EclipseUser.java b/src/main/java/org/eclipsefoundation/git/eca/api/models/EclipseUser.java
index d0d0e8c1..443bb8cf 100644
--- a/src/main/java/org/eclipsefoundation/git/eca/api/models/EclipseUser.java
+++ b/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);
diff --git a/src/test/java/org/eclipsefoundation/git/eca/resource/ValidationResourceTest.java b/src/test/java/org/eclipsefoundation/git/eca/resource/ValidationResourceTest.java
index 144bec78..1c2a56a0 100644
--- a/src/test/java/org/eclipsefoundation/git/eca/resource/ValidationResourceTest.java
+++ b/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);
diff --git a/src/test/java/org/eclipsefoundation/git/eca/service/impl/CachedUserServiceTest.java b/src/test/java/org/eclipsefoundation/git/eca/service/impl/CachedUserServiceTest.java
index cd2da33d..231b401c 100644
--- a/src/test/java/org/eclipsefoundation/git/eca/service/impl/CachedUserServiceTest.java
+++ b/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());
diff --git a/src/test/java/org/eclipsefoundation/git/eca/test/api/MockAccountsAPI.java b/src/test/java/org/eclipsefoundation/git/eca/test/api/MockAccountsAPI.java
index c6584a1e..445add9b 100644
--- a/src/test/java/org/eclipsefoundation/git/eca/test/api/MockAccountsAPI.java
+++ b/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);
}
}
--
GitLab