From a009cd4b7c236e2ac6b694b3a37b0782a66e1bbc Mon Sep 17 00:00:00 2001
From: Martin Lowe <martin.lowe@eclipse-foundation.org>
Date: Wed, 6 Jul 2022 14:46:42 -0400
Subject: [PATCH] Iss #69 - Fixed issue where validation messages could be lost
There was a very rare issue where spam inserting could cause issues
where sometimes a message was dropped when attempting to validate too
quickly. This was caused by manual FK management. Updates were done to
the status dto to tell Hibernate to manage the entity relations for us
using cascade + orphan removal.
Additional fixes were made to reduce the churn of objects with repeated
requests by reusing old validation messages if they already existed.
---
.../git/eca/dto/CommitValidationMessage.java | 54 ++++++++++++++++++-
.../git/eca/dto/CommitValidationStatus.java | 6 ++-
.../impl/DefaultValidationService.java | 45 ++++++++++------
3 files changed, 85 insertions(+), 20 deletions(-)
diff --git a/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationMessage.java b/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationMessage.java
index b9cde187..9d1267af 100644
--- a/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationMessage.java
+++ b/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationMessage.java
@@ -1,5 +1,9 @@
package org.eclipsefoundation.git.eca.dto;
+import java.util.List;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
import javax.inject.Inject;
import javax.inject.Singleton;
import javax.persistence.Entity;
@@ -27,7 +31,7 @@ public class CommitValidationMessage extends BareNode {
public static final DtoTable TABLE = new DtoTable(CommitValidationMessage.class, "cvm");
@Id
- @GeneratedValue(strategy=GenerationType.IDENTITY)
+ @GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
@ManyToOne
private CommitValidationStatus commit;
@@ -120,6 +124,47 @@ public class CommitValidationMessage extends BareNode {
this.authorEmail = authorEmail;
}
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = super.hashCode();
+ result = prime * result + Objects.hash(authorEmail, commit, eclipseId, id, providerId, statusCode);
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj)
+ return true;
+ if (!super.equals(obj))
+ return false;
+ if (getClass() != obj.getClass())
+ return false;
+ CommitValidationMessage other = (CommitValidationMessage) obj;
+ return Objects.equals(authorEmail, other.authorEmail) && Objects.equals(commit, other.commit)
+ && Objects.equals(eclipseId, other.eclipseId) && Objects.equals(id, other.id)
+ && Objects.equals(providerId, other.providerId) && statusCode == other.statusCode;
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder builder = new StringBuilder();
+ builder.append("CommitValidationMessage [id=");
+ builder.append(id);
+ builder.append(", commit=");
+ builder.append(commit.getCommitHash());
+ builder.append(", statusCode=");
+ builder.append(statusCode);
+ builder.append(", eclipseId=");
+ builder.append(eclipseId);
+ builder.append(", authorEmail=");
+ builder.append(authorEmail);
+ builder.append(", providerId=");
+ builder.append(providerId);
+ builder.append("]");
+ return builder.toString();
+ }
+
@Singleton
public static class CommitValidationMessageFilter implements DtoFilter<CommitValidationMessage> {
@Inject
@@ -141,6 +186,13 @@ public class CommitValidationMessage extends BareNode {
stmt.addClause(new ParameterizedSQLStatement.Clause(TABLE.getAlias() + ".commitId = ?",
new Object[] { Integer.valueOf(commitId) }));
}
+ // ids check
+ List<String> ids = params.get(DefaultUrlParameterNames.IDS.getName());
+ if (ids != null && !ids.isEmpty()) {
+ stmt.addClause(new ParameterizedSQLStatement.Clause(TABLE.getAlias() + ".id IN ?",
+ new Object[] { ids.stream().filter(StringUtils::isNumeric).map(Long::valueOf)
+ .collect(Collectors.toList()) }));
+ }
}
return stmt;
}
diff --git a/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationStatus.java b/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationStatus.java
index b9271f5b..d7022753 100644
--- a/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationStatus.java
+++ b/src/main/java/org/eclipsefoundation/git/eca/dto/CommitValidationStatus.java
@@ -5,9 +5,11 @@ import java.util.List;
import javax.inject.Inject;
import javax.inject.Singleton;
+import javax.persistence.CascadeType;
import javax.persistence.Entity;
import javax.persistence.EnumType;
import javax.persistence.Enumerated;
+import javax.persistence.FetchType;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
@@ -39,7 +41,7 @@ public class CommitValidationStatus extends BareNode {
private ProviderType provider;
private ZonedDateTime creationDate;
private ZonedDateTime lastModified;
- @OneToMany(mappedBy = "commit")
+ @OneToMany(mappedBy = "commit", fetch = FetchType.EAGER,cascade = { CascadeType.ALL }, orphanRemoval = true)
private List<CommitValidationMessage> errors;
@Override
@@ -167,7 +169,7 @@ public class CommitValidationStatus extends BareNode {
builder.append(", lastModified=");
builder.append(lastModified);
builder.append(", messages=");
- builder.append(errors);
+ builder.append(errors.toString());
builder.append("]");
return builder.toString();
}
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 230bc2e4..83cc07b4 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
@@ -81,8 +81,8 @@ public class DefaultValidationService implements ValidationService {
continue;
}
// update the status if present, otherwise make new one.
- Optional<CommitValidationStatus> status = statuses.stream().filter(s -> e.getKey().equals(s.getCommitHash()))
- .findFirst();
+ Optional<CommitValidationStatus> status = statuses.stream()
+ .filter(s -> e.getKey().equals(s.getCommitHash())).findFirst();
CommitValidationStatus base;
if (status.isPresent()) {
base = status.get();
@@ -99,21 +99,35 @@ public class DefaultValidationService implements ValidationService {
// get the commit for current status
Optional<Commit> commit = req.getCommits().stream().filter(c -> c.getHash().equals(e.getKey())).findFirst();
if (commit.isEmpty()) {
- // TODO
+ LOGGER.error("Could not find request commit associated with commit messages for commit hash '{}'",
+ e.getKey());
+ continue;
}
Commit c = commit.get();
// if there are errors, update validation messages
- if (e.getValue().getErrors().size() > 0) {
- messages.addAll(e.getValue().getErrors().stream().map(err -> {
- CommitValidationMessage m = new CommitValidationMessage();
- m.setAuthorEmail(c.getAuthor().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()));
+ if (e.getValue().getErrors().size() > 0 || (base.getErrors() != null && base.getErrors().size() > 0)) {
+ // 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.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);
}
}
String fingerprint = generateRequestHash(req);
@@ -121,8 +135,5 @@ public class DefaultValidationService implements ValidationService {
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()));
- // if present, remove any current validation messages as we will be pushing new ones
- dao.delete(q);
- dao.add(new RDBMSQuery<>(wrapper, filters.get(CommitValidationMessage.class)), messages);
}
}
--
GitLab