Skip to content
Snippets Groups Projects
Commit a009cd4b authored by Martin Lowe's avatar Martin Lowe :flag_ca:
Browse files

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.
parent 04fcf8fe
No related branches found
No related tags found
1 merge request!86Iss #69 - Fixed issue where validation messages could be lost
package org.eclipsefoundation.git.eca.dto; 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.Inject;
import javax.inject.Singleton; import javax.inject.Singleton;
import javax.persistence.Entity; import javax.persistence.Entity;
...@@ -27,7 +31,7 @@ public class CommitValidationMessage extends BareNode { ...@@ -27,7 +31,7 @@ public class CommitValidationMessage extends BareNode {
public static final DtoTable TABLE = new DtoTable(CommitValidationMessage.class, "cvm"); public static final DtoTable TABLE = new DtoTable(CommitValidationMessage.class, "cvm");
@Id @Id
@GeneratedValue(strategy=GenerationType.IDENTITY) @GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id; private Long id;
@ManyToOne @ManyToOne
private CommitValidationStatus commit; private CommitValidationStatus commit;
...@@ -120,6 +124,47 @@ public class CommitValidationMessage extends BareNode { ...@@ -120,6 +124,47 @@ public class CommitValidationMessage extends BareNode {
this.authorEmail = authorEmail; 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 @Singleton
public static class CommitValidationMessageFilter implements DtoFilter<CommitValidationMessage> { public static class CommitValidationMessageFilter implements DtoFilter<CommitValidationMessage> {
@Inject @Inject
...@@ -141,6 +186,13 @@ public class CommitValidationMessage extends BareNode { ...@@ -141,6 +186,13 @@ public class CommitValidationMessage extends BareNode {
stmt.addClause(new ParameterizedSQLStatement.Clause(TABLE.getAlias() + ".commitId = ?", stmt.addClause(new ParameterizedSQLStatement.Clause(TABLE.getAlias() + ".commitId = ?",
new Object[] { Integer.valueOf(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; return stmt;
} }
......
...@@ -5,9 +5,11 @@ import java.util.List; ...@@ -5,9 +5,11 @@ import java.util.List;
import javax.inject.Inject; import javax.inject.Inject;
import javax.inject.Singleton; import javax.inject.Singleton;
import javax.persistence.CascadeType;
import javax.persistence.Entity; import javax.persistence.Entity;
import javax.persistence.EnumType; import javax.persistence.EnumType;
import javax.persistence.Enumerated; import javax.persistence.Enumerated;
import javax.persistence.FetchType;
import javax.persistence.GeneratedValue; import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType; import javax.persistence.GenerationType;
import javax.persistence.Id; import javax.persistence.Id;
...@@ -39,7 +41,7 @@ public class CommitValidationStatus extends BareNode { ...@@ -39,7 +41,7 @@ public class CommitValidationStatus extends BareNode {
private ProviderType provider; private ProviderType provider;
private ZonedDateTime creationDate; private ZonedDateTime creationDate;
private ZonedDateTime lastModified; private ZonedDateTime lastModified;
@OneToMany(mappedBy = "commit") @OneToMany(mappedBy = "commit", fetch = FetchType.EAGER,cascade = { CascadeType.ALL }, orphanRemoval = true)
private List<CommitValidationMessage> errors; private List<CommitValidationMessage> errors;
@Override @Override
...@@ -167,7 +169,7 @@ public class CommitValidationStatus extends BareNode { ...@@ -167,7 +169,7 @@ public class CommitValidationStatus extends BareNode {
builder.append(", lastModified="); builder.append(", lastModified=");
builder.append(lastModified); builder.append(lastModified);
builder.append(", messages="); builder.append(", messages=");
builder.append(errors); builder.append(errors.toString());
builder.append("]"); builder.append("]");
return builder.toString(); return builder.toString();
} }
......
...@@ -81,8 +81,8 @@ public class DefaultValidationService implements ValidationService { ...@@ -81,8 +81,8 @@ public class DefaultValidationService implements ValidationService {
continue; continue;
} }
// update the status if present, otherwise make new one. // update the status if present, otherwise make new one.
Optional<CommitValidationStatus> status = statuses.stream().filter(s -> e.getKey().equals(s.getCommitHash())) Optional<CommitValidationStatus> status = statuses.stream()
.findFirst(); .filter(s -> e.getKey().equals(s.getCommitHash())).findFirst();
CommitValidationStatus base; CommitValidationStatus base;
if (status.isPresent()) { if (status.isPresent()) {
base = status.get(); base = status.get();
...@@ -99,21 +99,35 @@ public class DefaultValidationService implements ValidationService { ...@@ -99,21 +99,35 @@ public class DefaultValidationService implements ValidationService {
// get the commit for current status // get the commit for current status
Optional<Commit> commit = req.getCommits().stream().filter(c -> c.getHash().equals(e.getKey())).findFirst(); Optional<Commit> commit = req.getCommits().stream().filter(c -> c.getHash().equals(e.getKey())).findFirst();
if (commit.isEmpty()) { 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(); Commit c = commit.get();
// if there are errors, update validation messages // if there are errors, update validation messages
if (e.getValue().getErrors().size() > 0) { if (e.getValue().getErrors().size() > 0 || (base.getErrors() != null && base.getErrors().size() > 0)) {
messages.addAll(e.getValue().getErrors().stream().map(err -> { // generate new errors, looking for errors not found in current list
CommitValidationMessage m = new CommitValidationMessage(); List<CommitValidationMessage> currentErrors = base.getErrors() != null ? base.getErrors()
m.setAuthorEmail(c.getAuthor().getMail()); : new ArrayList<>();
m.setStatusCode(err.getCode().getValue()); List<CommitValidationMessage> newErrors = e.getValue().getErrors().stream().filter(
// TODO add a checked way to set this err -> currentErrors.stream().noneMatch(ce -> ce.getStatusCode() == err.getCode().getValue()))
m.setEclipseId(null); .map(err -> {
m.setProviderId(null); CommitValidationMessage m = new CommitValidationMessage();
m.setCommit(base); m.setAuthorEmail(c.getAuthor().getMail());
return m; m.setStatusCode(err.getCode().getValue());
}).collect(Collectors.toList())); // 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); String fingerprint = generateRequestHash(req);
...@@ -121,8 +135,5 @@ public class DefaultValidationService implements ValidationService { ...@@ -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(CommitValidationStatus.class)), updatedStatuses);
dao.add(new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatusGrouping.class)), updatedStatuses.stream() dao.add(new RDBMSQuery<>(wrapper, filters.get(CommitValidationStatusGrouping.class)), updatedStatuses.stream()
.map(s -> new CommitValidationStatusGrouping(fingerprint, s)).collect(Collectors.toList())); .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);
} }
} }
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment