From 57e62485dce6d028022e34292fbc6c55e18a302a Mon Sep 17 00:00:00 2001 From: Martin Lowe <martin.lowe@eclipse-foundation.org> Date: Mon, 11 Sep 2023 12:03:05 -0400 Subject: [PATCH 1/4] Iss #139 - Change when validation happens for GH status UI endpoint Previously, validation happened on every call to this endpoint. That led to issues with some of our calls where things were being revalidated when it wasn't needed. This would lead to failures to display, when the data should be able to be presented as is. Additionally, refactoring was done to split the validation status calls to a separate service to keep the code cleaner and more legible. --- .../git/eca/helper/CommitHelper.java | 31 ++- .../eca/helper/GithubValidationHelper.java | 5 +- .../git/eca/resource/StatusResource.java | 57 +++-- .../git/eca/service/ValidationService.java | 71 +----- .../eca/service/ValidationStatusService.java | 69 ++++++ .../impl/DefaultValidationService.java | 156 +------------- .../impl/DefaultValidationStatusService.java | 185 ++++++++++++++++ .../git/eca/helper/CommitHelperTest.java | 164 ++++++++++++-- .../impl/DefaultValidationServiceTest.java | 202 ------------------ .../DefaultValidationStatusServiceTest.java | 166 ++++++++++++++ 10 files changed, 654 insertions(+), 452 deletions(-) create mode 100644 src/main/java/org/eclipsefoundation/git/eca/service/ValidationStatusService.java create mode 100644 src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusService.java delete mode 100644 src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationServiceTest.java create mode 100644 src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusServiceTest.java diff --git a/src/main/java/org/eclipsefoundation/git/eca/helper/CommitHelper.java b/src/main/java/org/eclipsefoundation/git/eca/helper/CommitHelper.java index eaa36924..64b0c77f 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/helper/CommitHelper.java +++ b/src/main/java/org/eclipsefoundation/git/eca/helper/CommitHelper.java @@ -11,17 +11,22 @@ **********************************************************************/ package org.eclipsefoundation.git.eca.helper; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.List; import java.util.stream.Collectors; import javax.ws.rs.core.MultivaluedMap; +import org.eclipsefoundation.core.exception.ApplicationException; import org.eclipsefoundation.efservices.api.models.Project; import org.eclipsefoundation.git.eca.model.Commit; import org.eclipsefoundation.git.eca.model.ValidationRequest; import org.eclipsefoundation.git.eca.namespace.GitEcaParameterNames; import org.jboss.resteasy.specimpl.MultivaluedMapImpl; +import io.undertow.util.HexConverter; + /** * Contains helpers for processing commits. * @@ -63,8 +68,7 @@ public class CommitHelper { req.getRepoUrl().toString()); } - public static MultivaluedMap<String, String> getCommitParams(List<String> commitShas, String projectId, - String repoUrl) { + public static MultivaluedMap<String, String> getCommitParams(List<String> commitShas, String projectId, String repoUrl) { MultivaluedMap<String, String> params = new MultivaluedMapImpl<>(); params.put(GitEcaParameterNames.SHAS_RAW, commitShas); params.add(GitEcaParameterNames.REPO_URL_RAW, repoUrl); @@ -75,8 +79,27 @@ public class CommitHelper { } /** - * Centralized way of retrieving a checked project ID from a project for use - * when interacting with commits and commit data. + * Generates a request fingerprint for looking up requests that have already been processed in the past. Collision here + * is extremely unlikely, and low risk on the change it does. For that reason, a more secure but heavier hashing alg. + * wasn't chosen. + * + * @param req the request to generate a fingerprint for + * @return the fingerprint for the request + */ + public static String generateRequestHash(ValidationRequest req) { + StringBuilder sb = new StringBuilder(); + sb.append(req.getRepoUrl()); + req.getCommits().forEach(c -> sb.append(c.getHash())); + try { + return HexConverter.convertToHexString(MessageDigest.getInstance("MD5").digest(sb.toString().getBytes())); + } catch (NoSuchAlgorithmException e) { + throw new ApplicationException("Error while encoding request fingerprint - couldn't find MD5 algorithm.", e); + } + } + + /** + * Centralized way of retrieving a checked project ID from a project for use when interacting with commits and commit + * data. * * @param p the current project to attempt to retrieve an ID from * @return the project ID or the given default (empty string). diff --git a/src/main/java/org/eclipsefoundation/git/eca/helper/GithubValidationHelper.java b/src/main/java/org/eclipsefoundation/git/eca/helper/GithubValidationHelper.java index 58f9f2a2..f5ec3697 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/helper/GithubValidationHelper.java +++ b/src/main/java/org/eclipsefoundation/git/eca/helper/GithubValidationHelper.java @@ -52,6 +52,7 @@ import org.eclipsefoundation.git.eca.namespace.GithubCommitStatuses; import org.eclipsefoundation.git.eca.namespace.ProviderType; import org.eclipsefoundation.git.eca.service.GithubApplicationService; import org.eclipsefoundation.git.eca.service.ValidationService; +import org.eclipsefoundation.git.eca.service.ValidationStatusService; import org.eclipsefoundation.persistence.dao.PersistenceDao; import org.eclipsefoundation.persistence.model.RDBMSQuery; import org.eclipsefoundation.persistence.service.FilterService; @@ -87,6 +88,8 @@ public class GithubValidationHelper { @Inject ValidationService validation; @Inject + ValidationStatusService validationStatus; + @Inject GithubApplicationService ghAppService; @RestClient @@ -136,7 +139,7 @@ public class GithubValidationHelper { } // get the commit status of commits to use for checking historic validation - List<CommitValidationStatus> statuses = validation.getHistoricValidationStatusByShas(wrapper, commitShas); + List<CommitValidationStatus> statuses = validationStatus.getHistoricValidationStatusByShas(wrapper, commitShas); if (!"open".equalsIgnoreCase(prResponse.get().getState()) && statuses.isEmpty()) { throw new BadRequestException("Cannot find validation history for current non-open PR, cannot provide validation status"); } diff --git a/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java b/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java index aeef6297..0854d58e 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java +++ b/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java @@ -15,6 +15,7 @@ import java.util.List; import java.util.stream.Collectors; import javax.inject.Inject; +import javax.ws.rs.BadRequestException; import javax.ws.rs.GET; import javax.ws.rs.NotFoundException; import javax.ws.rs.Path; @@ -23,13 +24,18 @@ import javax.ws.rs.Produces; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.StringUtils; import org.eclipsefoundation.efservices.api.models.Project; import org.eclipsefoundation.git.eca.dto.CommitValidationStatus; import org.eclipsefoundation.git.eca.helper.GithubValidationHelper; import org.eclipsefoundation.git.eca.helper.ProjectHelper; +import org.eclipsefoundation.git.eca.model.Commit; +import org.eclipsefoundation.git.eca.model.ValidationRequest; import org.eclipsefoundation.git.eca.model.ValidationResponse; +import org.eclipsefoundation.git.eca.namespace.ProviderType; import org.eclipsefoundation.git.eca.service.GithubApplicationService; import org.eclipsefoundation.git.eca.service.ValidationService; +import org.eclipsefoundation.git.eca.service.ValidationStatusService; import io.quarkus.qute.Location; import io.quarkus.qute.Template; @@ -47,6 +53,8 @@ public class StatusResource extends GithubAdjacentResource { GithubApplicationService ghAppService; @Inject ValidationService validation; + @Inject + ValidationStatusService validationStatus; @Inject GithubValidationHelper validationHelper; @@ -66,7 +74,7 @@ public class StatusResource extends GithubAdjacentResource { @GET @Path("{fingerprint}") public Response getCommitValidation(@PathParam("fingerprint") String fingerprint) { - return Response.ok(validation.getHistoricValidationStatus(wrapper, fingerprint)).build(); + return Response.ok(validationStatus.getHistoricValidationStatus(wrapper, fingerprint)).build(); } /** @@ -80,7 +88,7 @@ public class StatusResource extends GithubAdjacentResource { @Produces(MediaType.TEXT_HTML) @Path("{fingerprint}/ui") public Response getCommitValidationUI(@PathParam("fingerprint") String fingerprint) { - List<CommitValidationStatus> statuses = validation.getHistoricValidationStatus(wrapper, fingerprint); + List<CommitValidationStatus> statuses = validationStatus.getHistoricValidationStatus(wrapper, fingerprint); if (statuses.isEmpty()) { throw new NotFoundException(String.format("Fingerprint '%s' not found", fingerprint)); } @@ -112,24 +120,49 @@ public class StatusResource extends GithubAdjacentResource { @Path("gh/{org}/{repoName}/{prNo}") public Response getCommitValidationForGithub(@PathParam("org") String org, @PathParam("repoName") String repoName, @PathParam("prNo") Integer prNo) { - // run the validation for the current request - ValidationResponse r = validationHelper.validateIncomingRequest(wrapper, org, repoName, prNo); + String repoFullName = org + '/' + repoName; + // check that the passed repo has a valid installation + String installationId = ghAppService.getInstallationForRepo(repoFullName); + if (StringUtils.isBlank(installationId)) { + throw new BadRequestException("Repo " + repoFullName + " requested, but does not have visible installation, returning"); + } + + // generate the URL used to retrieve valid projects + String repoUrl = GithubValidationHelper.getRepoUrl(org, repoName); + // get projects for use in queries + UI + List<Project> ps = projects.retrieveProjectsForRepoURL(GithubValidationHelper.getRepoUrl(org, repoName), ProviderType.GITHUB); + String projectName = null; + if (!ps.isEmpty()) { + projectName = ps.get(0).getProjectId(); + } + + // get the data about the current request, and check that we have some validation statuses + ValidationRequest req = validationHelper.generateRequest(projectName, repoFullName, prNo, repoUrl); + List<CommitValidationStatus> statuses = validationStatus + .getHistoricValidationStatusByShas(wrapper, req.getCommits().stream().map(Commit::getHash).collect(Collectors.toList())); + // check if we have any data, and if there is none, attempt to validate the request information + if (statuses.isEmpty()) { + // run the validation for the current request + ValidationResponse r = validationHelper.validateIncomingRequest(wrapper, org, repoName, prNo); + if (r == null) { + throw new BadRequestException("Cannot validate request for " + repoFullName + "#" + prNo + " as it is already closed"); + } + + // retrieve the status of the commits to display on the status page + statuses = validationStatus + .getHistoricValidationStatusByShas(wrapper, r.getCommits().keySet().stream().collect(Collectors.toList())); + } - // retrieve the status of the commits to display on the status page - List<CommitValidationStatus> statuses = validation - .getHistoricValidationStatusByShas(wrapper, r.getCommits().keySet().stream().collect(Collectors.toList())); - // get projects for use in status UI - List<Project> ps = projects.retrieveProjectsForRepoURL(statuses.get(0).getRepoUrl(), statuses.get(0).getProvider()); // render and return the status UI return Response .ok() .entity(statusUiTemplate .data("statuses", statuses) .data("pullRequestNumber", prNo) - .data("fullRepoName", org + '/' + repoName) + .data("fullRepoName", repoFullName) .data("project", ps.isEmpty() ? null : ps.get(0)) - .data("repoUrl", GithubValidationHelper.getRepoUrl(org, repoName)) - .data("installationId", ghAppService.getInstallationForRepo(org + '/' + repoName)) + .data("repoUrl", repoUrl) + .data("installationId", ghAppService.getInstallationForRepo(repoFullName)) .render()) .build(); } diff --git a/src/main/java/org/eclipsefoundation/git/eca/service/ValidationService.java b/src/main/java/org/eclipsefoundation/git/eca/service/ValidationService.java index ff230764..dc75ec9a 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/service/ValidationService.java +++ b/src/main/java/org/eclipsefoundation/git/eca/service/ValidationService.java @@ -11,21 +11,12 @@ **********************************************************************/ package org.eclipsefoundation.git.eca.service; -import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; -import java.util.List; - -import org.eclipsefoundation.core.exception.ApplicationException; import org.eclipsefoundation.core.model.RequestWrapper; -import org.eclipsefoundation.efservices.api.models.Project; -import org.eclipsefoundation.git.eca.dto.CommitValidationStatus; import org.eclipsefoundation.git.eca.model.ValidationRequest; import org.eclipsefoundation.git.eca.model.ValidationResponse; -import io.undertow.util.HexConverter; - /** - * Service containing logic for validating commits, and retrieving/updating previous validation statuses. + * Service containing logic for validating commits. * * @author Martin Lowe * @@ -42,64 +33,4 @@ public interface ValidationService { */ public ValidationResponse validateIncomingRequest(ValidationRequest req, RequestWrapper wrapper); - /** - * Retrieves a set of validation status objects given the validation request fingerprint. - * - * @param wrapper current request wrapper object - * @param fingerprint the validation request fingerprint - * @return the list of historic validation status objects, or an empty list. - */ - public List<CommitValidationStatus> getHistoricValidationStatus(RequestWrapper wrapper, String fingerprint); - - /** - * Retrieves a set of validation status objects given the target shas. - * - * @param wrapper current request wrapper object - * @param shas list of shas to use when fetching historic commit statuses - * @return the list of historic validation status objects, or an empty list. - */ - public List<CommitValidationStatus> getHistoricValidationStatusByShas(RequestWrapper wrapper, List<String> shas); - - /** - * Retrieves a set of commit validation status objects given a validation request and target project. - * - * @param wrapper current request wrapper object - * @param req the current validation request - * @param projectId the project targeted by the validation request - * @return the list of existing validation status objects for the validation request, or an empty list. - */ - public List<CommitValidationStatus> getRequestCommitValidationStatus(RequestWrapper wrapper, ValidationRequest req, String projectId); - - /** - * Updates or creates validation status objects for the commits validated as part of the current validation request. - * Uses information from both the original request and the final response to generate details to be preserved in commit - * status objects. - * - * @param wrapper current request wrapper object - * @param r the final validation response - * @param req the current validation request - * @param statuses list of existing commit validation objects to update - * @param p the project targeted by the validation request. - */ - public void updateCommitValidationStatus(RequestWrapper wrapper, ValidationResponse r, ValidationRequest req, - List<CommitValidationStatus> statuses, Project p); - - /** - * Generates a request fingerprint for looking up requests that have already been processed in the past. Collision here - * is extremely unlikely, and low risk on the change it does. For that reason, a more secure but heavier hashing alg. - * wasn't chosen. - * - * @param req the request to generate a fingerprint for - * @return the fingerprint for the request - */ - default String generateRequestHash(ValidationRequest req) { - StringBuilder sb = new StringBuilder(); - sb.append(req.getRepoUrl()); - req.getCommits().forEach(c -> sb.append(c.getHash())); - try { - return HexConverter.convertToHexString(MessageDigest.getInstance("MD5").digest(sb.toString().getBytes())); - } catch (NoSuchAlgorithmException e) { - throw new ApplicationException("Error while encoding request fingerprint - couldn't find MD5 algorithm.", e); - } - } } diff --git a/src/main/java/org/eclipsefoundation/git/eca/service/ValidationStatusService.java b/src/main/java/org/eclipsefoundation/git/eca/service/ValidationStatusService.java new file mode 100644 index 00000000..193f5455 --- /dev/null +++ b/src/main/java/org/eclipsefoundation/git/eca/service/ValidationStatusService.java @@ -0,0 +1,69 @@ +/** + * Copyright (c) 2023 Eclipse Foundation + * + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * Author: Martin Lowe <martin.lowe@eclipse-foundation.org> + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.eclipsefoundation.git.eca.service; + +import java.util.List; + +import org.eclipsefoundation.core.model.RequestWrapper; +import org.eclipsefoundation.efservices.api.models.Project; +import org.eclipsefoundation.git.eca.dto.CommitValidationStatus; +import org.eclipsefoundation.git.eca.model.ValidationRequest; +import org.eclipsefoundation.git.eca.model.ValidationResponse; + +/** + * Interface for retrieving/updating validation statuses. + */ +public interface ValidationStatusService { + + /** + * Retrieves a set of validation status objects given the validation request fingerprint. + * + * @param wrapper current request wrapper object + * @param fingerprint the validation request fingerprint + * @return the list of historic validation status objects, or an empty list. + */ + public List<CommitValidationStatus> getHistoricValidationStatus(RequestWrapper wrapper, String fingerprint); + + /** + * Retrieves a set of validation status objects given the target shas. + * + * @param wrapper current request wrapper object + * @param shas list of shas to use when fetching historic commit statuses + * @return the list of historic validation status objects, or an empty list. + */ + public List<CommitValidationStatus> getHistoricValidationStatusByShas(RequestWrapper wrapper, List<String> shas); + + /** + * Retrieves a set of commit validation status objects given a validation request and target project. + * + * @param wrapper current request wrapper object + * @param req the current validation request + * @param projectId the project targeted by the validation request + * @return the list of existing validation status objects for the validation request, or an empty list. + */ + public List<CommitValidationStatus> getRequestCommitValidationStatus(RequestWrapper wrapper, ValidationRequest req, String projectId); + + /** + * Updates or creates validation status objects for the commits validated as part of the current validation request. + * Uses information from both the original request and the final response to generate details to be preserved in commit + * status objects. + * + * @param wrapper current request wrapper object + * @param r the final validation response + * @param req the current validation request + * @param statuses list of existing commit validation objects to update + * @param p the project targeted by the validation request. + */ + public void updateCommitValidationStatus(RequestWrapper wrapper, ValidationResponse r, ValidationRequest req, + List<CommitValidationStatus> statuses, Project p); + +} 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 6e548ae0..e331e255 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 @@ -11,45 +11,34 @@ **********************************************************************/ package org.eclipsefoundation.git.eca.service.impl; -import java.util.ArrayList; -import java.util.Collections; import java.util.List; -import java.util.Map.Entry; import java.util.Optional; -import java.util.stream.Collectors; import javax.enterprise.context.ApplicationScoped; import javax.inject.Inject; import javax.ws.rs.WebApplicationException; -import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; import org.apache.commons.lang3.StringUtils; -import org.eclipsefoundation.core.helper.DateTimeHelper; import org.eclipsefoundation.core.model.RequestWrapper; import org.eclipsefoundation.core.service.CachingService; import org.eclipsefoundation.efservices.api.models.Project; import org.eclipsefoundation.git.eca.api.models.EclipseUser; import org.eclipsefoundation.git.eca.config.MailValidationConfig; -import org.eclipsefoundation.git.eca.dto.CommitValidationMessage; import org.eclipsefoundation.git.eca.dto.CommitValidationStatus; -import org.eclipsefoundation.git.eca.dto.CommitValidationStatusGrouping; import org.eclipsefoundation.git.eca.helper.CommitHelper; import org.eclipsefoundation.git.eca.helper.ProjectHelper; import org.eclipsefoundation.git.eca.model.Commit; -import org.eclipsefoundation.git.eca.model.CommitStatus; import org.eclipsefoundation.git.eca.model.GitUser; import org.eclipsefoundation.git.eca.model.ValidationRequest; import org.eclipsefoundation.git.eca.model.ValidationResponse; import org.eclipsefoundation.git.eca.namespace.APIStatusCode; -import org.eclipsefoundation.git.eca.namespace.GitEcaParameterNames; import org.eclipsefoundation.git.eca.service.UserService; import org.eclipsefoundation.git.eca.service.ValidationService; +import org.eclipsefoundation.git.eca.service.ValidationStatusService; import org.eclipsefoundation.persistence.dao.PersistenceDao; -import org.eclipsefoundation.persistence.model.RDBMSQuery; import org.eclipsefoundation.persistence.service.FilterService; -import org.jboss.resteasy.specimpl.MultivaluedMapImpl; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -71,6 +60,8 @@ public class DefaultValidationService implements ValidationService { ProjectHelper projects; @Inject UserService users; + @Inject + ValidationStatusService statusService; @Inject CachingService cache; @@ -89,7 +80,7 @@ public class DefaultValidationService implements ValidationService { .builder() .setStrictMode(Boolean.TRUE.equals(req.getStrictMode())) .setTrackedProject(!filteredProjects.isEmpty()) - .setFingerprint(generateRequestHash(req)) + .setFingerprint(CommitHelper.generateRequestHash(req)) .build(); // check to make sure commits are valid if (req.getCommits().stream().anyMatch(c -> !CommitHelper.validateCommit(c))) { @@ -105,8 +96,8 @@ public class DefaultValidationService implements ValidationService { } // get previous validation status messages - List<CommitValidationStatus> statuses = getRequestCommitValidationStatus(wrapper, req, - filteredProjects.isEmpty() ? null : filteredProjects.get(0).getProjectId()); + List<CommitValidationStatus> statuses = statusService + .getRequestCommitValidationStatus(wrapper, req, filteredProjects.isEmpty() ? null : filteredProjects.get(0).getProjectId()); req.getCommits().stream().forEach(c -> { // get the current status if present @@ -119,138 +110,10 @@ public class DefaultValidationService implements ValidationService { // process the current commit processCommit(c, r, filteredProjects); }); - updateCommitValidationStatus(wrapper, r, req, statuses, filteredProjects.isEmpty() ? null : filteredProjects.get(0)); + statusService.updateCommitValidationStatus(wrapper, r, req, statuses, filteredProjects.isEmpty() ? null : filteredProjects.get(0)); return r; } - @Override - public List<CommitValidationStatus> getHistoricValidationStatus(RequestWrapper wrapper, String fingerprint) { - if (StringUtils.isAllBlank(fingerprint)) { - return Collections.emptyList(); - } - MultivaluedMap<String, String> params = new MultivaluedMapImpl<>(); - params.add(GitEcaParameterNames.FINGERPRINT_RAW, fingerprint); - RDBMSQuery<CommitValidationStatusGrouping> q = new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatusGrouping.class), params); - // set use limit to false to collect all data in one request - q.setUseLimit(false); - return dao.get(q).stream().map(statusGrouping -> statusGrouping.getCompositeId().getCommit()).collect(Collectors.toList()); - } - - @Override - public List<CommitValidationStatus> getHistoricValidationStatusByShas(RequestWrapper wrapper, List<String> shas) { - if (shas == null || shas.isEmpty()) { - return Collections.emptyList(); - } - MultivaluedMap<String, String> params = new MultivaluedMapImpl<>(); - params.put(GitEcaParameterNames.SHAS_RAW, shas); - RDBMSQuery<CommitValidationStatus> q = new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatus.class), params); - // set use limit to false to collect all data in one request - q.setUseLimit(false); - return dao.get(q); - } - - @Override - public List<CommitValidationStatus> getRequestCommitValidationStatus(RequestWrapper wrapper, ValidationRequest req, String projectId) { - RDBMSQuery<CommitValidationStatus> q = new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatus.class), - CommitHelper.getCommitParams(req, projectId)); - // set use limit to false to collect all data in one request - q.setUseLimit(false); - return dao.get(q); - } - - @Override - public void updateCommitValidationStatus(RequestWrapper wrapper, ValidationResponse r, ValidationRequest req, - List<CommitValidationStatus> statuses, Project p) { - // iterate over commit responses, and update statuses in DB - List<CommitValidationStatus> updatedStatuses = r - .getCommits() - .entrySet() - .stream() - .filter(e -> !ValidationResponse.NIL_HASH_PLACEHOLDER.equalsIgnoreCase(e.getKey())) - .map(e -> recordUpdatedValidationStatus(req, statuses, p, e)) - .collect(Collectors.toList()); - String fingerprint = generateRequestHash(req); - // update the base commit status and messages - dao.add(new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatus.class)), updatedStatuses); - dao - .add(new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatusGrouping.class)), - updatedStatuses.stream().map(s -> new CommitValidationStatusGrouping(fingerprint, s)).collect(Collectors.toList())); - } - - /** - * Records new or updated validation status for passed commit entry. Checks existing records and updates where - * appropriate, otherwise creating new entires. - * - * @param req the complete validation request - * @param existingStatuses the statuses that may exist for the current request - * @param p the Eclipse project for the current request if it exists. - * @param e the current commit that is being updated - * @return the new or updated commit status - */ - private CommitValidationStatus recordUpdatedValidationStatus(ValidationRequest req, List<CommitValidationStatus> existingStatuses, - Project p, Entry<String, CommitStatus> e) { - // get the commit for current status - Optional<Commit> commit = req.getCommits().stream().filter(c -> e.getKey().equals(c.getHash())).findFirst(); - if (commit.isEmpty()) { - // this should always have a match (response commits are built from request commits) - LOGGER.error("Could not find request commit associated with commit messages for commit hash '{}'", e.getKey()); - return null; - } - Commit c = commit.get(); - // update the status if present, otherwise make new one. - Optional<CommitValidationStatus> status = existingStatuses.stream().filter(s -> e.getKey().equals(s.getCommitHash())).findFirst(); - CommitValidationStatus base; - if (status.isPresent()) { - base = status.get(); - } else { - base = new CommitValidationStatus(); - base.setProject(CommitHelper.getProjectId(p)); - base.setCommitHash(e.getKey()); - base.setUserMail(c.getAuthor().getMail()); - base.setProvider(req.getProvider()); - base.setRepoUrl(req.getRepoUrl().toString()); - base.setCreationDate(DateTimeHelper.now()); - base.setEstimatedLoc(req.getEstimatedLoc()); - } - base.setLastModified(DateTimeHelper.now()); - - // if there are errors, update validation messages - if (!e.getValue().getErrors().isEmpty() || (base.getErrors() != null && !base.getErrors().isEmpty())) { - // generate new errors, looking for errors not found in current list - List<CommitValidationMessage> currentErrors = base.getErrors() != null ? base.getErrors() : new ArrayList<>(); - List<CommitValidationMessage> newErrors = e - .getValue() - .getErrors() - .stream() - .filter(err -> currentErrors.stream().noneMatch(ce -> ce.getStatusCode() == err.getCode().getValue())) - .map(err -> { - CommitValidationMessage m = new CommitValidationMessage(); - m.setAuthorEmail(c.getAuthor().getMail()); - m.setCommitterEmail(c.getCommitter().getMail()); - m.setStatusCode(err.getCode().getValue()); - // TODO add a checked way to set this - m.setEclipseId(null); - m.setProviderId(null); - m.setCommit(base); - return m; - }) - .collect(Collectors.toList()); - LOGGER.debug("Encountered {} new errors for commit with hash '{}'", newErrors.size(), e.getKey()); - currentErrors.addAll(newErrors); - // remove errors that weren't encountered on this run - currentErrors - .removeIf(err -> e.getValue().getErrors().isEmpty() - || e.getValue().getErrors().stream().noneMatch(msg -> msg.getCode().getValue() == err.getStatusCode())); - LOGGER.trace("Encountered {} errors: {}", currentErrors.size(), currentErrors); - base.setErrors(currentErrors); - } - return base; - } - - /** - * REQUEST VALIDATION LOGIC - */ - /** * Process the current request, validating that the passed commit is valid. The author and committers Eclipse Account is * retrieved, which are then used to check if the current commit is valid for the current project. @@ -267,7 +130,8 @@ public class DefaultValidationService implements ValidationService { response.addMessage(c.getHash(), String.format("Authored by: %1$s <%2$s>", author.getName(), author.getMail())); // skip processing if a merge commit - if (c.getParents() != null && c.getParents().size() > 1) { + List<String> parents = c.getParents(); + if (parents != null && parents.size() > 1) { response .addMessage(c.getHash(), String.format("Commit '%1$s' has multiple parents, merge commit detected, passing", c.getHash())); @@ -419,7 +283,7 @@ public class DefaultValidationService implements ValidationService { } // check if user is a bot, either through early detection or through on-demand // check - if ((user.getIsBot() != null && user.getIsBot()) || users.userIsABot(user.getMail(), filteredProjects)) { + if ((user.getIsBot() != null && Boolean.TRUE.equals(user.getIsBot())) || users.userIsABot(user.getMail(), filteredProjects)) { LOGGER.debug("User '{} <{}>' was found to be a bot", user.getName(), user.getMail()); return true; } diff --git a/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusService.java b/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusService.java new file mode 100644 index 00000000..1913940b --- /dev/null +++ b/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusService.java @@ -0,0 +1,185 @@ +/** + * Copyright (c) 2023 Eclipse Foundation + * + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * Author: Martin Lowe <martin.lowe@eclipse-foundation.org> + * + * SPDX-License-Identifier: EPL-2.0 + */ +package org.eclipsefoundation.git.eca.service.impl; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map.Entry; +import java.util.Optional; +import java.util.stream.Collectors; + +import javax.enterprise.context.ApplicationScoped; +import javax.inject.Inject; +import javax.ws.rs.core.MultivaluedMap; + +import org.apache.commons.lang3.StringUtils; +import org.eclipsefoundation.core.helper.DateTimeHelper; +import org.eclipsefoundation.core.model.RequestWrapper; +import org.eclipsefoundation.core.service.CachingService; +import org.eclipsefoundation.efservices.api.models.Project; +import org.eclipsefoundation.git.eca.dto.CommitValidationMessage; +import org.eclipsefoundation.git.eca.dto.CommitValidationStatus; +import org.eclipsefoundation.git.eca.dto.CommitValidationStatusGrouping; +import org.eclipsefoundation.git.eca.helper.CommitHelper; +import org.eclipsefoundation.git.eca.model.Commit; +import org.eclipsefoundation.git.eca.model.CommitStatus; +import org.eclipsefoundation.git.eca.model.ValidationRequest; +import org.eclipsefoundation.git.eca.model.ValidationResponse; +import org.eclipsefoundation.git.eca.namespace.GitEcaParameterNames; +import org.eclipsefoundation.git.eca.service.ValidationStatusService; +import org.eclipsefoundation.persistence.dao.PersistenceDao; +import org.eclipsefoundation.persistence.model.RDBMSQuery; +import org.eclipsefoundation.persistence.service.FilterService; +import org.jboss.resteasy.specimpl.MultivaluedMapImpl; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * + */ +@ApplicationScoped +public class DefaultValidationStatusService implements ValidationStatusService { + private static final Logger LOGGER = LoggerFactory.getLogger(DefaultValidationStatusService.class); + + @Inject + CachingService cache; + @Inject + PersistenceDao dao; + @Inject + FilterService filters; + + @Override + public List<CommitValidationStatus> getHistoricValidationStatus(RequestWrapper wrapper, String fingerprint) { + if (StringUtils.isAllBlank(fingerprint)) { + return Collections.emptyList(); + } + MultivaluedMap<String, String> params = new MultivaluedMapImpl<>(); + params.add(GitEcaParameterNames.FINGERPRINT_RAW, fingerprint); + RDBMSQuery<CommitValidationStatusGrouping> q = new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatusGrouping.class), params); + // set use limit to false to collect all data in one request + q.setUseLimit(false); + return dao.get(q).stream().map(statusGrouping -> statusGrouping.getCompositeId().getCommit()).collect(Collectors.toList()); + } + + @Override + public List<CommitValidationStatus> getHistoricValidationStatusByShas(RequestWrapper wrapper, List<String> shas) { + if (shas == null || shas.isEmpty()) { + return Collections.emptyList(); + } + MultivaluedMap<String, String> params = new MultivaluedMapImpl<>(); + params.put(GitEcaParameterNames.SHAS_RAW, shas); + RDBMSQuery<CommitValidationStatus> q = new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatus.class), params); + // set use limit to false to collect all data in one request + q.setUseLimit(false); + return dao.get(q); + } + + @Override + public List<CommitValidationStatus> getRequestCommitValidationStatus(RequestWrapper wrapper, ValidationRequest req, String projectId) { + RDBMSQuery<CommitValidationStatus> q = new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatus.class), + CommitHelper.getCommitParams(req, projectId)); + // set use limit to false to collect all data in one request + q.setUseLimit(false); + return dao.get(q); + } + + @Override + public void updateCommitValidationStatus(RequestWrapper wrapper, ValidationResponse r, ValidationRequest req, + List<CommitValidationStatus> statuses, Project p) { + // iterate over commit responses, and update statuses in DB + List<CommitValidationStatus> updatedStatuses = r + .getCommits() + .entrySet() + .stream() + .filter(e -> !ValidationResponse.NIL_HASH_PLACEHOLDER.equalsIgnoreCase(e.getKey())) + .map(e -> recordUpdatedValidationStatus(req, statuses, p, e)) + .collect(Collectors.toList()); + String fingerprint = CommitHelper.generateRequestHash(req); + // update the base commit status and messages + dao.add(new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatus.class)), updatedStatuses); + dao + .add(new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatusGrouping.class)), + updatedStatuses.stream().map(s -> new CommitValidationStatusGrouping(fingerprint, s)).collect(Collectors.toList())); + } + + /** + * Records new or updated validation status for passed commit entry. Checks existing records and updates where + * appropriate, otherwise creating new entires. + * + * @param req the complete validation request + * @param existingStatuses the statuses that may exist for the current request + * @param p the Eclipse project for the current request if it exists. + * @param e the current commit that is being updated + * @return the new or updated commit status + */ + private CommitValidationStatus recordUpdatedValidationStatus(ValidationRequest req, List<CommitValidationStatus> existingStatuses, + Project p, Entry<String, CommitStatus> e) { + // get the commit for current status + Optional<Commit> commit = req.getCommits().stream().filter(c -> e.getKey().equals(c.getHash())).findFirst(); + if (commit.isEmpty()) { + // this should always have a match (response commits are built from request commits) + LOGGER.error("Could not find request commit associated with commit messages for commit hash '{}'", e.getKey()); + return null; + } + Commit c = commit.get(); + // update the status if present, otherwise make new one. + Optional<CommitValidationStatus> status = existingStatuses.stream().filter(s -> e.getKey().equals(s.getCommitHash())).findFirst(); + CommitValidationStatus base; + if (status.isPresent()) { + base = status.get(); + } else { + base = new CommitValidationStatus(); + base.setProject(CommitHelper.getProjectId(p)); + base.setCommitHash(e.getKey()); + base.setUserMail(c.getAuthor().getMail()); + base.setProvider(req.getProvider()); + base.setRepoUrl(req.getRepoUrl().toString()); + base.setCreationDate(DateTimeHelper.now()); + base.setEstimatedLoc(req.getEstimatedLoc()); + } + base.setLastModified(DateTimeHelper.now()); + + // if there are errors, update validation messages + if (!e.getValue().getErrors().isEmpty() || (base.getErrors() != null && !base.getErrors().isEmpty())) { + // generate new errors, looking for errors not found in current list + List<CommitValidationMessage> currentErrors = base.getErrors() != null ? base.getErrors() : new ArrayList<>(); + List<CommitValidationMessage> newErrors = e + .getValue() + .getErrors() + .stream() + .filter(err -> currentErrors.stream().noneMatch(ce -> ce.getStatusCode() == err.getCode().getValue())) + .map(err -> { + CommitValidationMessage m = new CommitValidationMessage(); + m.setAuthorEmail(c.getAuthor().getMail()); + m.setCommitterEmail(c.getCommitter().getMail()); + m.setStatusCode(err.getCode().getValue()); + // TODO add a checked way to set this + m.setEclipseId(null); + m.setProviderId(null); + m.setCommit(base); + return m; + }) + .collect(Collectors.toList()); + LOGGER.debug("Encountered {} new errors for commit with hash '{}'", newErrors.size(), e.getKey()); + currentErrors.addAll(newErrors); + // remove errors that weren't encountered on this run + currentErrors + .removeIf(err -> e.getValue().getErrors().isEmpty() + || e.getValue().getErrors().stream().noneMatch(msg -> msg.getCode().getValue() == err.getStatusCode())); + LOGGER.trace("Encountered {} errors: {}", currentErrors.size(), currentErrors); + base.setErrors(currentErrors); + } + return base; + } + +} diff --git a/src/test/java/org/eclipsefoundation/git/eca/helper/CommitHelperTest.java b/src/test/java/org/eclipsefoundation/git/eca/helper/CommitHelperTest.java index 982eb6ae..9678eda5 100644 --- a/src/test/java/org/eclipsefoundation/git/eca/helper/CommitHelperTest.java +++ b/src/test/java/org/eclipsefoundation/git/eca/helper/CommitHelperTest.java @@ -11,10 +11,16 @@ **********************************************************************/ package org.eclipsefoundation.git.eca.helper; +import java.net.URI; import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.UUID; import org.eclipsefoundation.git.eca.model.Commit; import org.eclipsefoundation.git.eca.model.GitUser; +import org.eclipsefoundation.git.eca.model.ValidationRequest; +import org.eclipsefoundation.git.eca.namespace.ProviderType; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -40,17 +46,20 @@ class CommitHelperTest { testUser = GitUser.builder().setMail("test.user@eclipse-foundation.org").setName("Tester McTesterson").build(); // basic known good commit - baseCommit = Commit.builder() - .setBody(String.format("Sample body content\n\nSigned-off-by: %s <%s>", testUser.getName(), - testUser.getMail())) - .setHash("abc123f").setHead(false).setParents(new ArrayList<>()) - .setSubject("Testing CommitHelper class #1337").setAuthor(testUser).setCommitter(testUser); + baseCommit = Commit + .builder() + .setBody(String.format("Sample body content\n\nSigned-off-by: %s <%s>", testUser.getName(), testUser.getMail())) + .setHash("abc123f") + .setHead(false) + .setParents(new ArrayList<>()) + .setSubject("Testing CommitHelper class #1337") + .setAuthor(testUser) + .setCommitter(testUser); } @Test void validateCommitKnownGood() { - Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), - "Expected basic commit to pass validation"); + Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), "Expected basic commit to pass validation"); } @Test @@ -61,35 +70,156 @@ class CommitHelperTest { @Test void validateCommitNoAuthorMail() { baseCommit.setAuthor(GitUser.builder().setName("Some Name").build()); - Assertions.assertFalse(CommitHelper.validateCommit(baseCommit.build()), - "Expected basic commit to fail validation w/ no author mail address"); + Assertions + .assertFalse(CommitHelper.validateCommit(baseCommit.build()), + "Expected basic commit to fail validation w/ no author mail address"); } @Test void validateCommitNoCommitterMail() { baseCommit.setCommitter(GitUser.builder().setName("Some Name").build()); - Assertions.assertFalse(CommitHelper.validateCommit(baseCommit.build()), - "Expected basic commit to fail validation w/ no committer mail address"); + Assertions + .assertFalse(CommitHelper.validateCommit(baseCommit.build()), + "Expected basic commit to fail validation w/ no committer mail address"); } @Test void validateCommitNoBody() { baseCommit.setBody(null); - Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), - "Expected basic commit to pass validation w/ no body"); + Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), "Expected basic commit to pass validation w/ no body"); } @Test void validateCommitNoParents() { baseCommit.setParents(new ArrayList<>()); - Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), - "Expected basic commit to pass validation w/ no parents"); + Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), "Expected basic commit to pass validation w/ no parents"); } @Test void validateCommitNoSubject() { baseCommit.setSubject(null); - Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), - "Expected basic commit to pass validation w/ no subject"); + Assertions.assertTrue(CommitHelper.validateCommit(baseCommit.build()), "Expected basic commit to pass validation w/ no subject"); + } + + @Test + void generateRequestHash_reproducible() { + ValidationRequest vr = generateBaseRequest(); + String fingerprint = CommitHelper.generateRequestHash(vr); + // if pushing the same set of commits without change the fingerprint won't change + Assertions.assertEquals(fingerprint, CommitHelper.generateRequestHash(vr)); + } + + @Test + void generateRequestHash_changesWithRepoURL() { + GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); + + List<Commit> commits = new ArrayList<>(); + // create sample commits + Commit c1 = Commit + .builder() + .setAuthor(g1) + .setCommitter(g1) + .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") + .setHash(UUID.randomUUID().toString()) + .setSubject("All of the things") + .setParents(Collections.emptyList()) + .build(); + commits.add(c1); + + // generate initial fingerprint + ValidationRequest vr = ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")) + .setCommits(commits) + .build(); + String fingerprint = CommitHelper.generateRequestHash(vr); + // generate request with different repo url + vr = ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/other-sample")) + .setCommits(commits) + .build(); + // fingerprint should change based on repo URL to reduce risk of collision + Assertions.assertNotEquals(fingerprint, CommitHelper.generateRequestHash(vr)); + } + + @Test + void generateRequestHash_changesWithNewCommits() { + GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); + List<Commit> commits = new ArrayList<>(); + // create sample commits + Commit c1 = Commit + .builder() + .setAuthor(g1) + .setCommitter(g1) + .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") + .setHash(UUID.randomUUID().toString()) + .setSubject("All of the things") + .setParents(Collections.emptyList()) + .build(); + commits.add(c1); + + // generate initial fingerprint + ValidationRequest vr = ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")) + .setCommits(commits) + .build(); + String fingerprint = CommitHelper.generateRequestHash(vr); + Commit c2 = Commit + .builder() + .setAuthor(g1) + .setCommitter(g1) + .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") + .setHash(UUID.randomUUID().toString()) + .setSubject("All of the things") + .setParents(Collections.emptyList()) + .build(); + commits.add(c2); + // generate request with different repo url + vr = ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/other-sample")) + .setCommits(commits) + .build(); + // each commit added should modify the fingerprint at least slightly + Assertions.assertNotEquals(fingerprint, CommitHelper.generateRequestHash(vr)); + } + + /** + * Used when a random validationRequest is needed and will not need to be recreated/modified. Base request should + * register as a commit for the test `sample.proj` project. + * + * @return random basic validation request. + */ + private ValidationRequest generateBaseRequest() { + GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); + List<Commit> commits = new ArrayList<>(); + // create sample commits + Commit c1 = Commit + .builder() + .setAuthor(g1) + .setCommitter(g1) + .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") + .setHash(UUID.randomUUID().toString()) + .setSubject("All of the things") + .setParents(Collections.emptyList()) + .build(); + commits.add(c1); + return ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")) + .setCommits(commits) + .build(); } } diff --git a/src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationServiceTest.java b/src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationServiceTest.java deleted file mode 100644 index 8b9d9d82..00000000 --- a/src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationServiceTest.java +++ /dev/null @@ -1,202 +0,0 @@ -/********************************************************************* -* Copyright (c) 2022 Eclipse Foundation. -* -* This program and the accompanying materials are made -* available under the terms of the Eclipse Public License 2.0 -* which is available at https://www.eclipse.org/legal/epl-2.0/ -* -* Author: Martin Lowe <martin.lowe@eclipse-foundation.org> -* -* SPDX-License-Identifier: EPL-2.0 -**********************************************************************/ -package org.eclipsefoundation.git.eca.service.impl; - -import java.net.URI; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.UUID; - -import javax.inject.Inject; - -import org.eclipsefoundation.core.model.FlatRequestWrapper; -import org.eclipsefoundation.core.model.RequestWrapper; -import org.eclipsefoundation.git.eca.dto.CommitValidationStatus; -import org.eclipsefoundation.git.eca.model.Commit; -import org.eclipsefoundation.git.eca.model.GitUser; -import org.eclipsefoundation.git.eca.model.ValidationRequest; -import org.eclipsefoundation.git.eca.namespace.ProviderType; -import org.eclipsefoundation.git.eca.service.ValidationService; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; - -import io.quarkus.test.junit.QuarkusTest; - -@QuarkusTest -class DefaultValidationServiceTest { - - @Inject - ValidationService validation; - - @Test - void generateRequestHash_reproducible() { - ValidationRequest vr = generateBaseRequest(); - String fingerprint = validation.generateRequestHash(vr); - // if pushing the same set of commits without change the fingerprint won't change - Assertions.assertEquals(fingerprint, validation.generateRequestHash(vr)); - } - - @Test - void generateRequestHash_changesWithRepoURL() { - GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); - - List<Commit> commits = new ArrayList<>(); - // create sample commits - Commit c1 = Commit.builder().setAuthor(g1).setCommitter(g1) - .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") - .setHash(UUID.randomUUID().toString()) - .setSubject("All of the things").setParents(Collections.emptyList()).build(); - commits.add(c1); - - // generate initial fingerprint - ValidationRequest vr = ValidationRequest.builder().setStrictMode(false).setProvider(ProviderType.GITHUB) - .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")).setCommits(commits) - .build(); - String fingerprint = validation.generateRequestHash(vr); - // generate request with different repo url - vr = ValidationRequest.builder().setStrictMode(false).setProvider(ProviderType.GITHUB) - .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/other-sample")) - .setCommits(commits).build(); - // fingerprint should change based on repo URL to reduce risk of collision - Assertions.assertNotEquals(fingerprint, validation.generateRequestHash(vr)); - } - - @Test - void generateRequestHash_changesWithNewCommits() { - GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); - List<Commit> commits = new ArrayList<>(); - // create sample commits - Commit c1 = Commit.builder().setAuthor(g1).setCommitter(g1) - .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") - .setHash(UUID.randomUUID().toString()) - .setSubject("All of the things").setParents(Collections.emptyList()).build(); - commits.add(c1); - - // generate initial fingerprint - ValidationRequest vr = ValidationRequest.builder().setStrictMode(false).setProvider(ProviderType.GITHUB) - .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")).setCommits(commits) - .build(); - String fingerprint = validation.generateRequestHash(vr); - Commit c2 = Commit.builder().setAuthor(g1).setCommitter(g1) - .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") - .setHash(UUID.randomUUID().toString()) - .setSubject("All of the things").setParents(Collections.emptyList()).build(); - commits.add(c2); - // generate request with different repo url - vr = ValidationRequest.builder().setStrictMode(false).setProvider(ProviderType.GITHUB) - .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/other-sample")) - .setCommits(commits).build(); - // each commit added should modify the fingerprint at least slightly - Assertions.assertNotEquals(fingerprint, validation.generateRequestHash(vr)); - } - - @Test - void getHistoricValidationStatus_noFingerprint() { - RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); - Assertions.assertTrue(validation.getHistoricValidationStatus(wrap, null).isEmpty()); - Assertions.assertTrue(validation.getHistoricValidationStatus(wrap, " ").isEmpty()); - Assertions.assertTrue(validation.getHistoricValidationStatus(wrap, "").isEmpty()); - } - - @Test - void getHistoricValidationStatus_noResultsForFingerprint() { - RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); - Assertions.assertTrue( - validation.getHistoricValidationStatus(wrap, UUID.randomUUID().toString()).isEmpty()); - } - - @Test - void getHistoricValidationStatus_success() { - RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); - Assertions.assertTrue(!validation.getHistoricValidationStatus(wrap, "957706b0f31e0ccfc5287c0ebc62dc79") - .isEmpty()); - } - - @Test - void getRequestCommitValidationStatus_noneExisting() { - RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); - ValidationRequest vr = generateBaseRequest(); - List<CommitValidationStatus> out = validation.getRequestCommitValidationStatus(wrap, vr, "sample.proj"); - // should always return non-null, should be empty w/ no results as there - // shouldn't be a matching status - Assertions.assertTrue(out.isEmpty()); - } - - @Test - void getRequestCommitValidationStatus_existing() { - // create request that lines up with one of the existing test commit validation statuses - RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); - GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); - List<Commit> commits = new ArrayList<>(); - // create sample commits - Commit c1 = Commit.builder().setAuthor(g1).setCommitter(g1) - .setBody("Signed-off-by: The Wizard <code.wiz@important.co>").setHash("123456789") - .setSubject("All of the things").setParents(Collections.emptyList()).build(); - commits.add(c1); - ValidationRequest vr = ValidationRequest.builder().setStrictMode(false).setProvider(ProviderType.GITHUB) - .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")).setCommits(commits) - .build(); - List<CommitValidationStatus> out = validation.getRequestCommitValidationStatus(wrap, vr, "sample.proj"); - // should contain one of the test status objects - Assertions.assertTrue(!out.isEmpty()); - } - - @Test - void getRequestCommitValidationStatus_noProjectWithResults() { - // create request that lines up with one of the existing test commit validation statuses - RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); - GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); - List<Commit> commits = new ArrayList<>(); - // create sample commits - Commit c1 = Commit.builder().setAuthor(g1).setCommitter(g1) - .setBody("Signed-off-by: The Wizard <code.wiz@important.co>").setHash("abc123def456") - .setSubject("All of the things").setParents(Collections.emptyList()).build(); - commits.add(c1); - ValidationRequest vr = ValidationRequest.builder().setStrictMode(false).setProvider(ProviderType.GITHUB) - .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")).setCommits(commits) - .build(); - List<CommitValidationStatus> out = validation.getRequestCommitValidationStatus(wrap, vr, null); - // should contain one of the test status objects - Assertions.assertTrue(!out.isEmpty()); - } - - @Test - void getRequestCommitValidationStatus_noProjectWithNoResults() { - RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); - ValidationRequest vr = generateBaseRequest(); - List<CommitValidationStatus> out = validation.getRequestCommitValidationStatus(wrap, vr, null); - // should contain one of the test status objects - Assertions.assertTrue(out.isEmpty()); - } - - /** - * Used when a random validationRequest is needed and will not need to be - * recreated/modified. Base request should register as a commit for the test - * `sample.proj` project. - * - * @return random basic validation request. - */ - private ValidationRequest generateBaseRequest() { - GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); - List<Commit> commits = new ArrayList<>(); - // create sample commits - Commit c1 = Commit.builder().setAuthor(g1).setCommitter(g1) - .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") - .setHash(UUID.randomUUID().toString()) - .setSubject("All of the things").setParents(Collections.emptyList()).build(); - commits.add(c1); - return ValidationRequest.builder().setStrictMode(false).setProvider(ProviderType.GITHUB) - .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")).setCommits(commits) - .build(); - } -} diff --git a/src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusServiceTest.java b/src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusServiceTest.java new file mode 100644 index 00000000..10d25077 --- /dev/null +++ b/src/test/java/org/eclipsefoundation/git/eca/service/impl/DefaultValidationStatusServiceTest.java @@ -0,0 +1,166 @@ +/********************************************************************* +* Copyright (c) 2022 Eclipse Foundation. +* +* This program and the accompanying materials are made +* available under the terms of the Eclipse Public License 2.0 +* which is available at https://www.eclipse.org/legal/epl-2.0/ +* +* Author: Martin Lowe <martin.lowe@eclipse-foundation.org> +* +* SPDX-License-Identifier: EPL-2.0 +**********************************************************************/ +package org.eclipsefoundation.git.eca.service.impl; + +import java.net.URI; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.UUID; + +import javax.inject.Inject; + +import org.eclipsefoundation.core.model.FlatRequestWrapper; +import org.eclipsefoundation.core.model.RequestWrapper; +import org.eclipsefoundation.git.eca.dto.CommitValidationStatus; +import org.eclipsefoundation.git.eca.model.Commit; +import org.eclipsefoundation.git.eca.model.GitUser; +import org.eclipsefoundation.git.eca.model.ValidationRequest; +import org.eclipsefoundation.git.eca.namespace.ProviderType; +import org.eclipsefoundation.git.eca.service.ValidationStatusService; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import io.quarkus.test.junit.QuarkusTest; + +@QuarkusTest +class DefaultValidationStatusServiceTest { + + @Inject + ValidationStatusService validationStatus; + + @Test + void getHistoricValidationStatus_noFingerprint() { + RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); + Assertions.assertTrue(validationStatus.getHistoricValidationStatus(wrap, null).isEmpty()); + Assertions.assertTrue(validationStatus.getHistoricValidationStatus(wrap, " ").isEmpty()); + Assertions.assertTrue(validationStatus.getHistoricValidationStatus(wrap, "").isEmpty()); + } + + @Test + void getHistoricValidationStatus_noResultsForFingerprint() { + RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); + Assertions.assertTrue(validationStatus.getHistoricValidationStatus(wrap, UUID.randomUUID().toString()).isEmpty()); + } + + @Test + void getHistoricValidationStatus_success() { + RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); + Assertions.assertTrue(!validationStatus.getHistoricValidationStatus(wrap, "957706b0f31e0ccfc5287c0ebc62dc79").isEmpty()); + } + + @Test + void getRequestCommitValidationStatus_noneExisting() { + RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); + ValidationRequest vr = generateBaseRequest(); + List<CommitValidationStatus> out = validationStatus.getRequestCommitValidationStatus(wrap, vr, "sample.proj"); + // should always return non-null, should be empty w/ no results as there + // shouldn't be a matching status + Assertions.assertTrue(out.isEmpty()); + } + + @Test + void getRequestCommitValidationStatus_existing() { + // create request that lines up with one of the existing test commit validation statuses + RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); + GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); + List<Commit> commits = new ArrayList<>(); + // create sample commits + Commit c1 = Commit + .builder() + .setAuthor(g1) + .setCommitter(g1) + .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") + .setHash("123456789") + .setSubject("All of the things") + .setParents(Collections.emptyList()) + .build(); + commits.add(c1); + ValidationRequest vr = ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")) + .setCommits(commits) + .build(); + List<CommitValidationStatus> out = validationStatus.getRequestCommitValidationStatus(wrap, vr, "sample.proj"); + // should contain one of the test status objects + Assertions.assertTrue(!out.isEmpty()); + } + + @Test + void getRequestCommitValidationStatus_noProjectWithResults() { + // create request that lines up with one of the existing test commit validation statuses + RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); + GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); + List<Commit> commits = new ArrayList<>(); + // create sample commits + Commit c1 = Commit + .builder() + .setAuthor(g1) + .setCommitter(g1) + .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") + .setHash("abc123def456") + .setSubject("All of the things") + .setParents(Collections.emptyList()) + .build(); + commits.add(c1); + ValidationRequest vr = ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")) + .setCommits(commits) + .build(); + List<CommitValidationStatus> out = validationStatus.getRequestCommitValidationStatus(wrap, vr, null); + // should contain one of the test status objects + Assertions.assertTrue(!out.isEmpty()); + } + + @Test + void getRequestCommitValidationStatus_noProjectWithNoResults() { + RequestWrapper wrap = new FlatRequestWrapper(URI.create("http://localhost/git/eca")); + ValidationRequest vr = generateBaseRequest(); + List<CommitValidationStatus> out = validationStatus.getRequestCommitValidationStatus(wrap, vr, null); + // should contain one of the test status objects + Assertions.assertTrue(out.isEmpty()); + } + + /** + * Used when a random validationRequest is needed and will not need to be recreated/modified. Base request should + * register as a commit for the test `sample.proj` project. + * + * @return random basic validation request. + */ + private ValidationRequest generateBaseRequest() { + GitUser g1 = GitUser.builder().setName("The Wizard").setMail("code.wiz@important.co").build(); + List<Commit> commits = new ArrayList<>(); + // create sample commits + Commit c1 = Commit + .builder() + .setAuthor(g1) + .setCommitter(g1) + .setBody("Signed-off-by: The Wizard <code.wiz@important.co>") + .setHash(UUID.randomUUID().toString()) + .setSubject("All of the things") + .setParents(Collections.emptyList()) + .build(); + commits.add(c1); + return ValidationRequest + .builder() + .setStrictMode(false) + .setProvider(ProviderType.GITHUB) + .setRepoUrl(URI.create("http://www.github.com/eclipsefdn/sample")) + .setCommits(commits) + .build(); + } +} -- GitLab From 72bbc07a6bd214c247857319997d3d54d2f7f18f Mon Sep 17 00:00:00 2001 From: Martin Lowe <martin.lowe@eclipse-foundation.org> Date: Mon, 11 Sep 2023 13:30:59 -0400 Subject: [PATCH 2/4] Add endpoint to check what repos are managed by which installation --- .../eca/resource/GithubWebhooksResource.java | 10 +++++++ .../eca/service/GithubApplicationService.java | 9 +++++++ .../impl/DefaultGithubApplicationService.java | 27 ++++++++++++------- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/eclipsefoundation/git/eca/resource/GithubWebhooksResource.java b/src/main/java/org/eclipsefoundation/git/eca/resource/GithubWebhooksResource.java index c7a6e176..14a33db9 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/resource/GithubWebhooksResource.java +++ b/src/main/java/org/eclipsefoundation/git/eca/resource/GithubWebhooksResource.java @@ -18,11 +18,14 @@ import java.util.Optional; import javax.inject.Inject; import javax.ws.rs.BadRequestException; import javax.ws.rs.FormParam; +import javax.ws.rs.GET; import javax.ws.rs.NotFoundException; import javax.ws.rs.POST; import javax.ws.rs.Path; +import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; import javax.ws.rs.ServerErrorException; +import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import org.apache.http.HttpStatus; @@ -100,6 +103,13 @@ public class GithubWebhooksResource extends GithubAdjacentResource { return Response.ok().build(); } + @GET + @Path("installations") + @Produces(MediaType.APPLICATION_JSON) + public Response getManagedInstallations() { + return Response.ok(ghAppService.getManagedInstallations()).build(); + } + /** * Endpoint for triggering revalidation of a past request. Uses the fingerprint hash to lookup the unique request and to * rebuild the request and revalidate if it exists. diff --git a/src/main/java/org/eclipsefoundation/git/eca/service/GithubApplicationService.java b/src/main/java/org/eclipsefoundation/git/eca/service/GithubApplicationService.java index 3a36f46c..4b6cafab 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/service/GithubApplicationService.java +++ b/src/main/java/org/eclipsefoundation/git/eca/service/GithubApplicationService.java @@ -13,6 +13,8 @@ package org.eclipsefoundation.git.eca.service; import java.util.Optional; +import javax.ws.rs.core.MultivaluedMap; + import org.eclipsefoundation.git.eca.api.models.GithubWebhookRequest.PullRequest; /** @@ -23,6 +25,13 @@ import org.eclipsefoundation.git.eca.api.models.GithubWebhookRequest.PullRequest */ public interface GithubApplicationService { + /** + * Retrieve safe map of all installations that are managed by this instance of the Eclispe ECA app. + * + * @return map containing installation IDs mapped to the list of repos they support for the current app + */ + MultivaluedMap<String, String> getManagedInstallations(); + /** * Retrieves the installation ID for the ECA app on the given repo if it exists. * diff --git a/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultGithubApplicationService.java b/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultGithubApplicationService.java index 1b374c05..d1f04c33 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultGithubApplicationService.java +++ b/src/main/java/org/eclipsefoundation/git/eca/service/impl/DefaultGithubApplicationService.java @@ -12,6 +12,7 @@ package org.eclipsefoundation.git.eca.service.impl; import java.time.Duration; +import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.concurrent.TimeUnit; @@ -53,8 +54,8 @@ public class DefaultGithubApplicationService implements GithubApplicationService // cache kept small as there should only ever really be 1 entry, but room added for safety private static final Integer MAX_CACHE_SIZE = 10; - private static final Long INSTALL_REPO_FETCH_MAX_TIMEOUT = 10l; - + private static final Long INSTALL_REPO_FETCH_MAX_TIMEOUT = 15l; + @ConfigProperty(name = "eclipse.github.default-api-version", defaultValue = "2022-11-28") String apiVersion; @@ -85,12 +86,27 @@ public class DefaultGithubApplicationService implements GithubApplicationService getAllInstallRepos(); } + @Override + public MultivaluedMap<String, String> getManagedInstallations() { + // create a deep copy of the map to protect from modifications and return it + MultivaluedMap<String, String> mapClone = new MultivaluedMapImpl<>(); + getAllInstallRepos().entrySet().stream().forEach(e -> mapClone.put(e.getKey(), new ArrayList<>(e.getValue()))); + return mapClone; + } + @Override public String getInstallationForRepo(String repoFullName) { MultivaluedMap<String, String> map = getAllInstallRepos(); return map.keySet().stream().filter(k -> map.get(k).contains(repoFullName)).findFirst().orElse(null); } + @Override + public Optional<PullRequest> getPullRequest(String installationId, String repoFullName, Integer pullRequest) { + return cache + .get(repoFullName, new MultivaluedMapImpl<>(), PullRequest.class, + () -> gh.getPullRequest(jwt.getGhBearerString(installationId), apiVersion, repoFullName, pullRequest)); + } + private MultivaluedMap<String, String> getAllInstallRepos() { try { return this.installationRepositoriesCache.get("all").get(INSTALL_REPO_FETCH_MAX_TIMEOUT, TimeUnit.SECONDS); @@ -105,13 +121,6 @@ public class DefaultGithubApplicationService implements GithubApplicationService return new MultivaluedMapImpl<>(); } - @Override - public Optional<PullRequest> getPullRequest(String installationId, String repoFullName, Integer pullRequest) { - return cache - .get(repoFullName, new MultivaluedMapImpl<>(), PullRequest.class, - () -> gh.getPullRequest(jwt.getGhBearerString(installationId), apiVersion, repoFullName, pullRequest)); - } - /** * Retrieves a fresh copy of installation repositories, mapped by installation ID to associated full repo names. * -- GitLab From 0148636a2cb4d0c500d42bc1c8ca5d81857579c5 Mon Sep 17 00:00:00 2001 From: Martin Lowe <martin.lowe@eclipse-foundation.org> Date: Mon, 11 Sep 2023 13:54:57 -0400 Subject: [PATCH 3/4] Fix bad call using projectId instead of installation ID for GH status Call was using the project ID in place of the installation ID, leading to failures to retrieve the github access tokens. --- .../git/eca/resource/StatusResource.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java b/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java index 0854d58e..0db2c74d 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java +++ b/src/main/java/org/eclipsefoundation/git/eca/resource/StatusResource.java @@ -129,15 +129,9 @@ public class StatusResource extends GithubAdjacentResource { // generate the URL used to retrieve valid projects String repoUrl = GithubValidationHelper.getRepoUrl(org, repoName); - // get projects for use in queries + UI - List<Project> ps = projects.retrieveProjectsForRepoURL(GithubValidationHelper.getRepoUrl(org, repoName), ProviderType.GITHUB); - String projectName = null; - if (!ps.isEmpty()) { - projectName = ps.get(0).getProjectId(); - } // get the data about the current request, and check that we have some validation statuses - ValidationRequest req = validationHelper.generateRequest(projectName, repoFullName, prNo, repoUrl); + ValidationRequest req = validationHelper.generateRequest(installationId, repoFullName, prNo, repoUrl); List<CommitValidationStatus> statuses = validationStatus .getHistoricValidationStatusByShas(wrapper, req.getCommits().stream().map(Commit::getHash).collect(Collectors.toList())); // check if we have any data, and if there is none, attempt to validate the request information @@ -153,6 +147,8 @@ public class StatusResource extends GithubAdjacentResource { .getHistoricValidationStatusByShas(wrapper, r.getCommits().keySet().stream().collect(Collectors.toList())); } + // get projects for use in queries + UI + List<Project> ps = projects.retrieveProjectsForRepoURL(repoUrl, ProviderType.GITHUB); // render and return the status UI return Response .ok() -- GitLab From 22187f883bde68b1ffb3449c4cfdfe0d63f46db0 Mon Sep 17 00:00:00 2001 From: Martin Lowe <martin.lowe@eclipse-foundation.org> Date: Mon, 11 Sep 2023 14:04:31 -0400 Subject: [PATCH 4/4] Fix bad revalidation parameter name for repo_full_name --- .../git/eca/namespace/GitEcaParameterNames.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/eclipsefoundation/git/eca/namespace/GitEcaParameterNames.java b/src/main/java/org/eclipsefoundation/git/eca/namespace/GitEcaParameterNames.java index cbfaa0ee..70a657c1 100644 --- a/src/main/java/org/eclipsefoundation/git/eca/namespace/GitEcaParameterNames.java +++ b/src/main/java/org/eclipsefoundation/git/eca/namespace/GitEcaParameterNames.java @@ -35,7 +35,7 @@ public final class GitEcaParameterNames implements UrlParameterNamespace { public static final String STATUS_DELETED_RAW = "deleted"; public static final String SINCE_RAW = "since"; public static final String UNTIL_RAW = "until"; - public static final String REPOSITORY_FULL_NAME_RAW = "repository_full_name"; + public static final String REPOSITORY_FULL_NAME_RAW = "repo_full_name"; public static final String INSTALLATION_ID_RAW = "installation_id"; public static final String PULL_REQUEST_NUMBER_RAW = "pull_request_number"; public static final String USER_MAIL_RAW = "user_mail"; -- GitLab