diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/IssueUpdater.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/IssueUpdater.java index f90682a6c..aa4aa4b89 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/IssueUpdater.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/IssueUpdater.java @@ -356,6 +356,11 @@ public boolean isIdempotent() { return true; } + @Override + public String name() { + return "issue"; + } + @Override public void handleNewIssue(PullRequest pr, org.openjdk.skara.vcs.openjdk.Issue issue) { var realIssue = issueProject.issue(issue.id()); diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/JsonUpdater.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/JsonUpdater.java index 6b6b5b786..46982d4b0 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/JsonUpdater.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/JsonUpdater.java @@ -111,4 +111,9 @@ public void handleNewBranch(HostedRepository repository, Repository localReposit public boolean isIdempotent() { return false; } + + @Override + public String name() { + return "json"; + } } diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/MailingListUpdater.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/MailingListUpdater.java index c2d8fce07..16652ad77 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/MailingListUpdater.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/MailingListUpdater.java @@ -384,4 +384,9 @@ public void handleNewBranch(HostedRepository repository, Repository localReposit public boolean isIdempotent() { return false; } + + @Override + public String name() { + return "ml"; + } } diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryUpdateConsumer.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryUpdateConsumer.java index 526bc7480..f459ddf8d 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryUpdateConsumer.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryUpdateConsumer.java @@ -34,4 +34,5 @@ public interface RepositoryUpdateConsumer { void handleTagCommit(HostedRepository repository, Repository localRepository, Commit commit, Tag tag, Tag.Annotated annotation); void handleNewBranch(HostedRepository repository, Repository localRepository, List commits, Branch parent, Branch branch); boolean isIdempotent(); + String name(); } diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java index 86d85e4bb..eb560bdc3 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/RepositoryWorkItem.java @@ -54,7 +54,7 @@ public class RepositoryWorkItem implements WorkItem { this.updaters = updaters; } - private void handleNewRef(Repository localRepo, Reference ref, Collection allRefs, boolean runOnlyIdempotent) { + private void handleNewRef(Repository localRepo, Reference ref, Collection allRefs, RepositoryUpdateConsumer updater) { // Figure out the best parent ref var candidates = new HashSet<>(allRefs); candidates.remove(ref); @@ -77,55 +77,68 @@ private void handleNewRef(Repository localRepo, Reference ref, Collection bestParentCommits; try { - var bestParentCommits = localRepo.commits(bestParent.getKey().hash().hex() + ".." + ref.hash(), true); - for (var updater : updaters) { - if (updater.isIdempotent() != runOnlyIdempotent) { - continue; - } - var branch = new Branch(ref.name()); - var parent = new Branch(bestParent.getKey().name()); - updater.handleNewBranch(repository, localRepo, bestParentCommits.asList(), parent, branch); - } + bestParentCommits = localRepo.commits(bestParent.getKey().hash().hex() + ".." + ref.hash(), true).asList(); } catch (IOException e) { - e.printStackTrace(); + throw new UncheckedIOException(e); } + var branch = new Branch(ref.name()); + var parent = new Branch(bestParent.getKey().name()); + updater.handleNewBranch(repository, localRepo, bestParentCommits, parent, branch); } - private void handleUpdatedRef(Repository localRepo, Reference ref, List commits, boolean runOnlyIdempotent) { - for (var updater : updaters) { - if (updater.isIdempotent() != runOnlyIdempotent) { - continue; - } - var branch = new Branch(ref.name()); - updater.handleCommits(repository, localRepo, commits, branch); - } + private void handleUpdatedRef(Repository localRepo, Reference ref, List commits, RepositoryUpdateConsumer updater) { + var branch = new Branch(ref.name()); + updater.handleCommits(repository, localRepo, commits, branch); } - private void handleRef(Repository localRepo, UpdateHistory history, Reference ref, Collection allRefs) throws IOException { + private List handleRef(Repository localRepo, UpdateHistory history, Reference ref, Collection allRefs) throws IOException { + var errors = new ArrayList(); var branch = new Branch(ref.name()); - var lastHash = history.branchHash(branch); - if (lastHash.isEmpty()) { - log.warning("No previous history found for branch '" + branch + "' - resetting mark"); - handleNewRef(localRepo, ref, allRefs, true); - history.setBranchHash(branch, ref.hash()); - handleNewRef(localRepo, ref, allRefs, false); - } else { - var commitMetadata = localRepo.commitMetadata(lastHash.get() + ".." + ref.hash()); - if (commitMetadata.size() == 0) { - return; - } - if (commitMetadata.size() > 1000) { - history.setBranchHash(branch, ref.hash()); - throw new RuntimeException("Excessive amount of new commits on branch " + branch.name() + - " detected (" + commitMetadata.size() + ") - skipping notifications"); - } + for (var updater : updaters) { + var lastHash = history.branchHash(branch, updater.name()); + if (lastHash.isEmpty()) { + log.warning("No previous history found for branch '" + branch + "' and updater '" + updater.name() + " - resetting mark"); + if (!updater.isIdempotent()) { + history.setBranchHash(branch, updater.name(), ref.hash()); + } + try { + handleNewRef(localRepo, ref, allRefs, updater); + if (updater.isIdempotent()) { + history.setBranchHash(branch, updater.name(), ref.hash()); + } + } catch (RuntimeException e) { + errors.add(e); + } + } else { + var commitMetadata = localRepo.commitMetadata(lastHash.get() + ".." + ref.hash()); + if (commitMetadata.size() == 0) { + continue; + } + if (commitMetadata.size() > 1000) { + history.setBranchHash(branch, updater.name(), ref.hash()); + errors.add(new RuntimeException("Excessive amount of new commits on branch " + branch.name() + + " detected (" + commitMetadata.size() + ") for updater '" + + updater.name() + "' - skipping notifications")); + continue; + } - var commits = localRepo.commits(lastHash.get() + ".." + ref.hash(), true).asList(); - handleUpdatedRef(localRepo, ref, commits, true); - history.setBranchHash(branch, ref.hash()); - handleUpdatedRef(localRepo, ref, commits, false); + var commits = localRepo.commits(lastHash.get() + ".." + ref.hash(), true).asList(); + if (!updater.isIdempotent()) { + history.setBranchHash(branch, updater.name(), ref.hash()); + } + try { + handleUpdatedRef(localRepo, ref, commits, updater); + if (updater.isIdempotent()) { + history.setBranchHash(branch, updater.name(), ref.hash()); + } + } catch (RuntimeException e) { + errors.add(e); + } + } } + return errors; } private Optional existingPrevious(OpenJDKTag tag, Set allJdkTags) { @@ -276,17 +289,23 @@ public void run(Path scratchPath) { var history = UpdateHistory.create(tagStorageBuilder, historyPath.resolve("tags"), branchStorageBuilder, historyPath.resolve("branches")); handleTags(localRepo, history); - boolean hasBranchHistory = knownRefs.stream() - .map(ref -> history.branchHash(new Branch(ref.name()))) - .anyMatch(Optional::isPresent); + boolean hasBranchHistory = !history.isEmpty(); + var errors = new ArrayList(); for (var ref : knownRefs) { if (!hasBranchHistory) { - log.warning("No previous history found for any branch - resetting mark for '" + ref.name() + "'"); - history.setBranchHash(new Branch(ref.name()), ref.hash()); + log.warning("No previous history found for any branch - resetting mark for '" + ref.name()); + for (var updater : updaters) { + log.info("Resetting mark for branch '" + ref.name() + "' for updater '" + updater.name() + "'"); + history.setBranchHash(new Branch(ref.name()), updater.name(), ref.hash()); + } } else { - handleRef(localRepo, history, ref, knownRefs); + errors.addAll(handleRef(localRepo, history, ref, knownRefs)); } } + if (!errors.isEmpty()) { + errors.forEach(error -> log.throwing("RepositoryWorkItem", "run", error)); + throw new RuntimeException("Errors detected during ref updating"); + } } catch (IOException e) { throw new UncheckedIOException(e); } diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/ResolvedBranch.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/ResolvedBranch.java index 515ca6dc4..1b61b74cc 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/ResolvedBranch.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/ResolvedBranch.java @@ -28,10 +28,12 @@ class ResolvedBranch { private final Branch branch; + private final String updater; private final Hash hash; - ResolvedBranch(Branch branch, Hash hash) { + ResolvedBranch(Branch branch, String updater, Hash hash) { this.branch = branch; + this.updater = updater; this.hash = hash; } @@ -39,13 +41,12 @@ public Branch branch() { return branch; } - public Hash hash() { - return hash; + public String updater() { + return updater; } - @Override - public String toString() { - return branch.name() + ":" + hash().hex(); + public Hash hash() { + return hash; } @Override @@ -57,12 +58,11 @@ public boolean equals(Object o) { return false; } ResolvedBranch that = (ResolvedBranch) o; - return Objects.equals(branch, that.branch) && - Objects.equals(hash, that.hash); + return branch.equals(that.branch) && updater.equals(that.updater) && hash.equals(that.hash); } @Override public int hashCode() { - return Objects.hash(branch, hash); + return Objects.hash(branch, updater, hash); } } diff --git a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/UpdateHistory.java b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/UpdateHistory.java index 907265c0f..eb138e353 100644 --- a/bots/notify/src/main/java/org/openjdk/skara/bots/notify/UpdateHistory.java +++ b/bots/notify/src/main/java/org/openjdk/skara/bots/notify/UpdateHistory.java @@ -27,30 +27,44 @@ import java.nio.file.Path; import java.util.*; +import java.util.function.Function; import java.util.stream.*; class UpdateHistory { - private final Storage tagStorage; private final Storage branchStorage; - private Map branches; + private Map branchHashes; private Set tags; + private List parseSerializedEntry(String entry) { + var parts = entry.split(" "); + if (parts.length == 2) { + // Transform legacy entry + var issueEntry = new ResolvedBranch(new Branch(parts[0]), "issue", new Hash(parts[1])); + var mlEntry = new ResolvedBranch(new Branch(parts[0]), "ml", new Hash(parts[1])); + return List.of(issueEntry, mlEntry); + } + return List.of(new ResolvedBranch(new Branch(parts[0]), parts[1], new Hash(parts[2]))); + } + private Set loadBranches(String current) { return current.lines() - .map(line -> line.split(" ")) - .map(entry -> new ResolvedBranch(new Branch(entry[0]), new Hash(entry[1]))) + .flatMap(line -> parseSerializedEntry(line).stream()) .collect(Collectors.toSet()); } + private String serializeEntry(ResolvedBranch entry) { + return entry.branch().toString() + " " + entry.updater() + " " + entry.hash().toString(); + } + private String serializeBranches(Collection added, Set existing) { var updatedBranches = existing.stream() - .collect(Collectors.toMap(ResolvedBranch::branch, - ResolvedBranch::hash)); - added.forEach(a -> updatedBranches.put(a.branch(), a.hash())); - return updatedBranches.entrySet().stream() - .map(entry -> entry.getKey().toString() + " " + entry.getValue().toString()) + .collect(Collectors.toMap(entry -> entry.branch().toString() + ":" + entry.updater(), + Function.identity())); + added.forEach(a -> updatedBranches.put(a.branch().toString() + ":" + a.updater(), a)); + return updatedBranches.values().stream() + .map(this::serializeEntry) .sorted() .collect(Collectors.joining("\n")); } @@ -73,9 +87,9 @@ private Set currentTags() { return tagStorage.current(); } - private Map currentBranchHashes() { + private Map currentBranchHashes() { return branchStorage.current().stream() - .collect(Collectors.toMap(ResolvedBranch::branch, ResolvedBranch::hash)); + .collect(Collectors.toMap(rb -> rb.branch().toString() + " " + rb.updater(), ResolvedBranch::hash)); } private UpdateHistory(StorageBuilder tagStorageBuilder, Path tagLocation, StorageBuilder branchStorageBuilder, Path branchLocation) { @@ -90,7 +104,7 @@ private UpdateHistory(StorageBuilder tagStorageBuilder, Path tagLocation, S .materialize(branchLocation); tags = currentTags(); - branches = currentBranchHashes(); + branchHashes = currentBranchHashes(); } static UpdateHistory create(StorageBuilder tagStorageBuilder, Path tagLocation, StorageBuilder branchStorageBuilder, Path branchLocation) { @@ -116,25 +130,29 @@ boolean hasTag(Tag tag) { return tags.contains(tag); } - void setBranchHash(Branch branch, Hash hash) { - var entry = new ResolvedBranch(branch, hash); + void setBranchHash(Branch branch, String updater, Hash hash) { + var entry = new ResolvedBranch(branch, updater, hash); branchStorage.put(entry); var newBranchHashes = currentBranchHashes(); // Sanity check - if (branches != null) { - for (var existingBranch : branches.keySet()) { + if (branchHashes != null) { + for (var existingBranch : branchHashes.keySet()) { if (!newBranchHashes.containsKey(existingBranch)) { throw new RuntimeException("Hash information for branch '" + existingBranch + "' is missing"); } } } - branches = newBranchHashes; + branchHashes = newBranchHashes; + } + + Optional branchHash(Branch branch, String updater) { + var entry = branchHashes.get(branch.toString() + " " + updater); + return Optional.ofNullable(entry); } - Optional branchHash(Branch branch) { - var hash = branches.get(branch); - return Optional.ofNullable(hash); + boolean isEmpty() { + return branchHashes.isEmpty(); } } diff --git a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdateHistoryTests.java b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdateHistoryTests.java index 984d23b6b..9d1086a0f 100644 --- a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdateHistoryTests.java +++ b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdateHistoryTests.java @@ -84,17 +84,42 @@ void branchesRetained(TestInfo testInfo) throws IOException { var history = createHistory(repository, ref); - history.setBranchHash(new Branch("1"), new Hash("a")); - history.setBranchHash(new Branch("2"), new Hash("b")); - history.setBranchHash(new Branch("1"), new Hash("c")); + history.setBranchHash(new Branch("1"), "a", new Hash("a")); + history.setBranchHash(new Branch("2"), "a", new Hash("b")); + history.setBranchHash(new Branch("1"), "a", new Hash("c")); - assertEquals(new Hash("c"), history.branchHash(new Branch("1")).orElseThrow()); - assertEquals(new Hash("b"), history.branchHash(new Branch("2")).orElseThrow()); + assertEquals(new Hash("c"), history.branchHash(new Branch("1"), "a").orElseThrow()); + assertEquals(new Hash("b"), history.branchHash(new Branch("2"), "a").orElseThrow()); var newHistory = createHistory(repository, ref); - assertEquals(new Hash("c"), newHistory.branchHash(new Branch("1")).orElseThrow()); - assertEquals(new Hash("b"), newHistory.branchHash(new Branch("2")).orElseThrow()); + assertEquals(new Hash("c"), newHistory.branchHash(new Branch("1"), "a").orElseThrow()); + assertEquals(new Hash("b"), newHistory.branchHash(new Branch("2"), "a").orElseThrow()); + } + } + + @Test + void branchesSeparateUpdaters(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repository = credentials.getHostedRepository(); + var ref = resetHostedRepository(repository); + + var history = createHistory(repository, ref); + + history.setBranchHash(new Branch("1"), "a", new Hash("a")); + history.setBranchHash(new Branch("2"), "a", new Hash("b")); + history.setBranchHash(new Branch("1"), "b", new Hash("c")); + history.setBranchHash(new Branch("2"), "a", new Hash("d")); + + assertEquals(new Hash("a"), history.branchHash(new Branch("1"), "a").orElseThrow()); + assertEquals(new Hash("d"), history.branchHash(new Branch("2"), "a").orElseThrow()); + assertEquals(new Hash("c"), history.branchHash(new Branch("1"), "b").orElseThrow()); + + var newHistory = createHistory(repository, ref); + + assertEquals(new Hash("a"), newHistory.branchHash(new Branch("1"), "a").orElseThrow()); + assertEquals(new Hash("d"), newHistory.branchHash(new Branch("2"), "a").orElseThrow()); + assertEquals(new Hash("c"), newHistory.branchHash(new Branch("1"), "b").orElseThrow()); } } diff --git a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdaterTests.java b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdaterTests.java index c2404b387..def66edf9 100644 --- a/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdaterTests.java +++ b/bots/notify/src/test/java/org/openjdk/skara/bots/notify/UpdaterTests.java @@ -29,7 +29,9 @@ import org.openjdk.skara.mailinglist.MailingListServerFactory; import org.openjdk.skara.storage.StorageBuilder; import org.openjdk.skara.test.*; +import org.openjdk.skara.vcs.*; import org.openjdk.skara.vcs.Tag; +import org.openjdk.skara.vcs.openjdk.OpenJDKTag; import org.junit.jupiter.api.*; @@ -1911,4 +1913,111 @@ void testPullRequestPROnly(TestInfo testInfo) throws IOException { assertEquals(1, comments.size()); } } + + private static class TestRepositoryUpdateConsumer implements RepositoryUpdateConsumer { + private final String name; + private final boolean idempotent; + private int updateCount = 0; + private boolean shouldFail = false; + + TestRepositoryUpdateConsumer(String name, boolean idempotent) { + this.name = name; + this.idempotent = idempotent; + } + + @Override + public void handleCommits(HostedRepository repository, Repository localRepository, List commits, + Branch branch) { + updateCount++; + if (shouldFail) { + throw new RuntimeException("induced failure"); + } + } + + @Override + public void handleOpenJDKTagCommits(HostedRepository repository, Repository localRepository, + List commits, OpenJDKTag tag, Tag.Annotated annotated) { + throw new RuntimeException("unexpected"); + } + + @Override + public void handleTagCommit(HostedRepository repository, Repository localRepository, Commit commit, Tag tag, + Tag.Annotated annotation) { + throw new RuntimeException("unexpected"); + } + + @Override + public void handleNewBranch(HostedRepository repository, Repository localRepository, List commits, + Branch parent, Branch branch) { + throw new RuntimeException("unexpected"); + } + + @Override + public boolean isIdempotent() { + return idempotent; + } + + @Override + public String name() { + return name; + } + } + + @Test + void testIdempotenceMix(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var repo = credentials.getHostedRepository(); + var repoFolder = tempFolder.path().resolve("repo"); + var localRepo = CheckableRepository.init(repoFolder, repo.repositoryType()); + credentials.commitLock(localRepo); + localRepo.pushAll(repo.url()); + + var tagStorage = createTagStorage(repo); + var branchStorage = createBranchStorage(repo); + var prIssuesStorage = createPullRequestIssuesStorage(repo); + var storageFolder = tempFolder.path().resolve("storage"); + + var idempotent = new TestRepositoryUpdateConsumer("i", true); + var nonIdempotent = new TestRepositoryUpdateConsumer("ni", false); + var notifyBot = NotifyBot.newBuilder() + .repository(repo) + .storagePath(storageFolder) + .branches(Pattern.compile("master")) + .tagStorageBuilder(tagStorage) + .branchStorageBuilder(branchStorage) + .prIssuesStorageBuilder(prIssuesStorage) + .updaters(List.of(idempotent, nonIdempotent)) + .build(); + + // Initialize history + TestBotRunner.runPeriodicItems(notifyBot); + + // Create an issue and commit a fix + var editHash = CheckableRepository.appendAndCommit(localRepo, "Another line", "Fix stuff"); + localRepo.push(editHash, repo.url(), "master"); + TestBotRunner.runPeriodicItems(notifyBot); + + // Both updaters should have run + assertEquals(1, idempotent.updateCount); + assertEquals(1, nonIdempotent.updateCount); + + var nextEditHash = CheckableRepository.appendAndCommit(localRepo, "Yet another line", "Fix more stuff"); + localRepo.push(nextEditHash, repo.url(), "master"); + + idempotent.shouldFail = true; + nonIdempotent.shouldFail = true; + assertThrows(RuntimeException.class, () -> TestBotRunner.runPeriodicItems(notifyBot)); + + // Both updaters should have run again + assertEquals(2, idempotent.updateCount); + assertEquals(2, nonIdempotent.updateCount); + + assertThrows(RuntimeException.class, () -> TestBotRunner.runPeriodicItems(notifyBot)); + + // But now only the idempotent one should have been retried + assertEquals(3, idempotent.updateCount); + assertEquals(2, nonIdempotent.updateCount); + } + } }