diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AllowCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AllowCommand.java index 156dd9645..b8a54c70c 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AllowCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/AllowCommand.java @@ -31,11 +31,11 @@ public class AllowCommand implements CommandHandler { @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { var botUser = pr.repository().forge().currentUser(); var vetoers = Veto.vetoers(botUser, allComments); - if (!vetoers.contains(comment.author().id())) { + if (!vetoers.contains(command.user().id())) { reply.println("You have not rejected this change"); return; } @@ -48,7 +48,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst reply.println("However, it still remains blocked by other rejections."); } - reply.println(Veto.removeVeto(comment.author())); + reply.println(Veto.removeVeto(command.user())); } @Override diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java index 4e34f05f5..3dfe59d5a 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java @@ -56,22 +56,22 @@ private static void linkReply(PullRequest pr, Issue issue, PrintWriter writer) { } @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!censusInstance.isReviewer(comment.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!censusInstance.isReviewer(command.user())) { reply.println("only [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed require a CSR."); return; } var labels = pr.labels(); - if (args.trim().toLowerCase().equals("unneeded")) { + if (command.args().trim().toLowerCase().equals("unneeded")) { if (labels.contains(CSR_LABEL)) { pr.removeLabel(CSR_LABEL); } reply.println("determined that a [CSR](https://wiki.openjdk.java.net/display/csr/Main) request " + "is no longer needed for this pull request."); return; - } else if (!args.isEmpty()) { + } else if (!command.args().isEmpty()) { showHelp(reply); return; } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java index 663055a0f..9adb40115 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckWorkItem.java @@ -159,6 +159,6 @@ public Collection run(Path scratchPath) { throw new UncheckedIOException(e); } } - return List.of(); + return List.of(new CommandWorkItem(bot, pr, errorHandler)); } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandHandler.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandHandler.java index 9e0670156..7eeb94d45 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandHandler.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandHandler.java @@ -30,6 +30,13 @@ import java.util.List; interface CommandHandler { - void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply); + void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply); String description(); + + default boolean multiLine() { + return false; + } + default boolean allowedInBody() { + return false; + } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java new file mode 100644 index 000000000..aded111e2 --- /dev/null +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandInvocation.java @@ -0,0 +1,41 @@ +package org.openjdk.skara.bots.pr; + +import org.openjdk.skara.host.HostUser; + +import java.util.Optional; + +class CommandInvocation { + private final String id; + private final HostUser user; + private final CommandHandler handler; + private final String name; + private final String args; + + CommandInvocation(String id, HostUser user, CommandHandler handler, String name, String args) { + this.id = id; + this.user = user; + this.handler = handler; + this.name = name; + this.args = args != null ? args.strip() : ""; + } + + String id() { + return id; + } + + HostUser user() { + return user; + } + + Optional handler() { + return Optional.ofNullable(handler); + } + + String name() { + return name; + } + + String args() { + return args; + } +} diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java index c129a9d67..87bfb1fac 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/CommandWorkItem.java @@ -24,6 +24,7 @@ import org.openjdk.skara.bot.WorkItem; import org.openjdk.skara.forge.PullRequest; +import org.openjdk.skara.host.HostUser; import org.openjdk.skara.issuetracker.Comment; import java.io.*; @@ -37,7 +38,7 @@ public class CommandWorkItem extends PullRequestWorkItem { private static final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr"); - private static final Pattern commandPattern = Pattern.compile("^/(.*)"); + private static final Pattern commandPattern = Pattern.compile("^\\s*/([A-Za-z]+)(?:\\s+(.*))?"); private static final String commandReplyMarker = ""; private static final Pattern commandReplyPattern = Pattern.compile(""); private static final String selfCommandMarker = ""; @@ -61,14 +62,14 @@ static class HelpCommand implements CommandHandler { static private Map external = null; @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { reply.println("Available commands:"); Stream.concat( commandHandlers.entrySet().stream() .map(entry -> entry.getKey() + " - " + entry.getValue().description()), external.entrySet().stream() .map(entry -> entry.getKey() + " - " + entry.getValue()) - ).sorted().forEachOrdered(command -> reply.println(" * " + command)); + ).sorted().forEachOrdered(c -> reply.println(" * " + c)); } @Override @@ -99,27 +100,102 @@ private List> findCommandComments(List< .collect(Collectors.toList()); } - private void processCommand(PullRequest pr, CensusInstance censusInstance, Path scratchPath, String command, Comment comment, List allComments) { + private String formatId(String baseId, int subId) { + if (subId > 0) { + return String.format("%s:%d", baseId, subId); + } else { + return baseId; + } + } + + private static class InvalidBodyCommandHandler implements CommandHandler { + @Override + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + reply.println("The command `" + command.name() + "` cannot be used in the pull request body. Please use it in a new comment."); + } + + @Override + public String description() { + return ""; + } + } + + private List extractCommands(String text, String baseId, HostUser user) { + var ret = new ArrayList(); + CommandHandler multiLineHandler = null; + List multiLineBuffer = null; + String multiLineCommand = null; + int subId = 0; + for (var line : text.split("\\R")) { + var commandMatcher = commandPattern.matcher(line); + if (commandMatcher.matches()) { + if (multiLineHandler != null) { + ret.add(new CommandInvocation(formatId(baseId, subId++), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer))); + multiLineHandler = null; + } + var command = commandMatcher.group(1).toLowerCase(); + var handler = commandHandlers.get(command); + if (handler != null && baseId.equals("body") && !handler.allowedInBody()) { + handler = new InvalidBodyCommandHandler(); + } + if (handler != null && handler.multiLine()) { + multiLineHandler = handler; + multiLineBuffer = new ArrayList<>(); + if (commandMatcher.group(2) != null) { + multiLineBuffer.add(commandMatcher.group(2)); + } + multiLineCommand = command; + } else { + ret.add(new CommandInvocation(formatId(baseId, subId++), user, handler, command, commandMatcher.group(2))); + } + } else { + if (multiLineHandler != null) { + multiLineBuffer.add(line); + } + } + } + if (multiLineHandler != null) { + ret.add(new CommandInvocation(formatId(baseId, subId), user, multiLineHandler, multiLineCommand, String.join("\n", multiLineBuffer))); + } + return ret; + } + + private Optional nextCommand(PullRequest pr, List comments) { + var self = pr.repository().forge().currentUser(); + var allCommands = Stream.concat(extractCommands(pr.body(), "body", pr.author()).stream(), + comments.stream() + .filter(comment -> !comment.author().equals(self) || comment.body().endsWith(selfCommandMarker)) + .flatMap(c -> extractCommands(c.body(), c.id(), c.author()).stream())) + .collect(Collectors.toList()); + + var handled = comments.stream() + .filter(comment -> comment.author().equals(self)) + .map(comment -> commandReplyPattern.matcher(comment.body())) + .filter(Matcher::find) + .map(matcher -> matcher.group(1)) + .collect(Collectors.toSet()); + + return allCommands.stream() + .filter(ci -> !handled.contains(ci.id())) + .findFirst(); + } + + private void processCommand(PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments) { var writer = new StringWriter(); var printer = new PrintWriter(writer); - printer.println(String.format(commandReplyMarker, comment.id())); + printer.println(String.format(commandReplyMarker, command.id())); printer.print("@"); - printer.print(comment.author().userName()); + printer.print(command.user().userName()); printer.print(" "); - command = command.strip(); - var argSplit = command.indexOf(' '); - var commandWord = argSplit > 0 ? command.substring(0, argSplit) : command; - var commandArgs = argSplit > 0 ? command.substring(argSplit + 1) : ""; - - var handler = commandHandlers.get(commandWord); - if (handler != null) { - handler.handle(bot, pr, censusInstance, scratchPath, commandArgs, comment, allComments, printer); + var handler = command.handler(); + if (handler.isPresent()) { + handler.get().handle(bot, pr, censusInstance, scratchPath, command, allComments, printer); } else { - if (!bot.externalCommands().containsKey(commandWord)) { + if (!bot.externalCommands().containsKey(command.name())) { printer.print("Unknown command `"); - printer.print(command); + printer.print(command.name()); printer.println("` - for a list of valid commands use `/help`."); } else { // Do not reply to external commands @@ -132,7 +208,7 @@ private void processCommand(PullRequest pr, CensusInstance censusInstance, Path @Override public Collection run(Path scratchPath) { - log.info("Looking for merge commands"); + log.info("Looking for PR commands"); if (pr.labels().contains("integrated")) { log.info("Skip checking for commands in integrated PR"); @@ -140,9 +216,9 @@ public Collection run(Path scratchPath) { } var comments = pr.comments(); - var unprocessedCommands = findCommandComments(comments); - if (unprocessedCommands.isEmpty()) { - log.fine("No new merge commands found, stopping further processing"); + var nextCommand = nextCommand(pr, comments); + if (nextCommand.isEmpty()) { + log.fine("No new PR commands found, stopping further processing"); return List.of(); } @@ -151,9 +227,7 @@ public Collection run(Path scratchPath) { } var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr); - for (var entry : unprocessedCommands) { - processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), entry.getKey(), entry.getValue(), comments); - } + processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), nextCommand.get(), comments); // Run another check to reflect potential changes from commands return List.of(new CheckWorkItem(bot, pr, errorHandler)); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ContributorCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ContributorCommand.java index 545ee59c4..18ac433cf 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ContributorCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ContributorCommand.java @@ -79,13 +79,13 @@ private Optional parseUser(String user, PullRequest pr, CensusInst } @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!comment.author().equals(pr.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!command.user().equals(pr.author())) { reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `contributor` command."); return; } - var matcher = commandPattern.matcher(args); + var matcher = commandPattern.matcher(command.args()); if (!matcher.matches()) { showHelp(pr, reply); return; @@ -130,4 +130,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst public String description() { return "adds or removes additional contributors for a PR"; } + + @Override + public boolean allowedInBody() { + return true; + } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java index e35043972..e04aba221 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java @@ -53,14 +53,14 @@ private Optional checkProblem(Map performedChecks, String } @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!comment.author().equals(pr.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!command.user().equals(pr.author())) { reply.print("Only the author (@" + pr.author().userName() + ") is allowed to issue the `integrate` command."); // If the command author is allowed to sponsor this change, suggest that command var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments); if (readyHash.isPresent()) { - if (censusInstance.isCommitter(comment.author())) { + if (censusInstance.isCommitter(command.user())) { reply.print(" As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the `/sponsor` command?"); return; } @@ -94,8 +94,8 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst // Validate the target hash if requested var rebaseMessage = new StringWriter(); - if (!args.isBlank()) { - var wantedHash = new Hash(args); + if (!command.args().isBlank()) { + var wantedHash = new Hash(command.args()); if (!pr.targetHash().equals(wantedHash)) { reply.print("The head of the target branch is no longer at the requested hash " + wantedHash); reply.println(" - it has moved to " + pr.targetHash() + ". Aborting integration."); @@ -129,7 +129,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst if (!censusInstance.isCommitter(pr.author())) { reply.println(ReadyForSponsorTracker.addIntegrationMarker(pr.headHash())); reply.println("Your change (at version " + pr.headHash() + ") is now ready to be sponsored by a Committer."); - if (!args.isBlank()) { + if (!command.args().isBlank()) { reply.println("Note that your sponsor will make the final decision onto which target hash to integrate."); } pr.addLabel("sponsor"); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IssueCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IssueCommand.java index 31f62a005..aa943a4bd 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IssueCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/IssueCommand.java @@ -272,11 +272,12 @@ private void createIssue(PullRequestBot bot, PullRequest pr, String args, Census } @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!comment.author().equals(pr.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!command.user().equals(pr.author())) { reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `/" + name + "` command."); return; } + var args = command.args(); if (args.isBlank()) { showHelp(reply); return; @@ -295,7 +296,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst if (args.startsWith("remove") || args.startsWith("delete")) { removeIssue(bot, args, currentSolved, reply); } else if (args.startsWith("create")) { - createIssue(bot, pr, args, censusInstance, comment.author(), reply); + createIssue(bot, pr, args, censusInstance, command.user(), reply); } else { addIssue(bot, pr, args, currentSolved, reply); } @@ -308,4 +309,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst public String description() { return "edit the list of issues that this PR solves"; } + + @Override + public boolean allowedInBody() { + return true; + } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelCommand.java index 12c800626..454b4d418 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/LabelCommand.java @@ -59,13 +59,13 @@ private Set automaticLabels(PullRequestBot bot, PullRequest pr, Path scr } @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!comment.author().equals(pr.author()) && (!censusInstance.isCommitter(comment.author()))) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!command.user().equals(pr.author()) && (!censusInstance.isCommitter(command.user()))) { reply.println("Only the PR author and project [Committers](https://openjdk.java.net/bylaws#committer) are allowed to modify labels on a PR."); return; } - var argumentMatcher = argumentPattern.matcher(args); + var argumentMatcher = argumentPattern.matcher(command.args()); if (!argumentMatcher.matches()) { showHelp(bot.labelConfiguration(), reply); return; @@ -118,4 +118,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst public String description() { return "add or remove an additional classification label"; } + + @Override + public boolean allowedInBody() { + return true; + } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java index bebca84c2..0f330e818 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBot.java @@ -119,7 +119,6 @@ private List getWorkItems(List pullRequests) { } ret.add(new CheckWorkItem(this, pr, e -> updateCache.invalidate(pr))); - ret.add(new CommandWorkItem(this, pr, e -> updateCache.invalidate(pr))); ret.add(new LabelerWorkItem(this, pr, e -> updateCache.invalidate(pr))); } } diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/RejectCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/RejectCommand.java index 5d4183eec..8ed9c6949 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/RejectCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/RejectCommand.java @@ -31,12 +31,12 @@ public class RejectCommand implements CommandHandler { @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (pr.author().equals(comment.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (pr.author().equals(command.user())) { reply.println("You can't reject your own changes."); return; } - if (!censusInstance.isReviewer(comment.author())) { + if (!censusInstance.isReviewer(command.user())) { reply.println("Only [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed to reject changes."); return; } @@ -44,7 +44,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst var botUser = pr.repository().forge().currentUser(); var vetoers = Veto.vetoers(botUser, allComments); - if (vetoers.contains(comment.author().id())) { + if (vetoers.contains(command.user().id())) { reply.println("You have already rejected this change."); return; } @@ -55,7 +55,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst reply.println("This change cannot be integrated while the rejection is in place. To lift the rejection, "); reply.println("issue an allow command: `/allow`"); - reply.println(Veto.addVeto(comment.author())); + reply.println(Veto.addVeto(command.user())); if (vetoers.isEmpty()) { pr.addLabel("rejected"); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java index 39c698fe7..358fe96a3 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewerCommand.java @@ -66,8 +66,8 @@ private Optional parseUser(String user, PullRequest pr, CensusInsta } @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!comment.author().equals(pr.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!command.user().equals(pr.author())) { reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `reviewer` command."); return; } @@ -76,7 +76,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst return; } - var matcher = commandPattern.matcher(args); + var matcher = commandPattern.matcher(command.args()); if (!matcher.matches()) { showHelp(pr, reply); return; diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java index eb8d5d81a..031b0fd12 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/ReviewersCommand.java @@ -48,13 +48,13 @@ private void showHelp(PrintWriter reply) { } @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!censusInstance.isReviewer(comment.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!censusInstance.isReviewer(command.user())) { reply.println("Only [Reviewers](https://openjdk.java.net/bylaws#reviewer) are allowed to change the number of required reviewers."); return; } - var splitArgs = args.split(" "); + var splitArgs = command.args().split(" "); if (splitArgs.length < 1 || splitArgs.length > 2) { showHelp(reply); return; diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java index 330204708..aa95b086c 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SponsorCommand.java @@ -35,12 +35,12 @@ public class SponsorCommand implements CommandHandler { private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr"); @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { if (censusInstance.isCommitter(pr.author())) { reply.println("This change does not need sponsoring - the author is allowed to integrate it."); return; } - if (!censusInstance.isReviewer(comment.author())) { + if (!censusInstance.isReviewer(command.user())) { reply.println("Only [Committers](https://openjdk.java.net/bylaws#committer) are allowed to sponsor changes."); return; } @@ -79,8 +79,8 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst // Validate the target hash if requested var rebaseMessage = new StringWriter(); - if (!args.isBlank()) { - var wantedHash = new Hash(args); + if (!command.args().isBlank()) { + var wantedHash = new Hash(command.args()); if (!pr.targetHash().equals(wantedHash)) { reply.print("The head of the target branch is no longer at the requested hash " + wantedHash); reply.println(" - it has moved to " + pr.targetHash() + ". Aborting integration."); @@ -97,7 +97,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst } var localHash = checkablePr.commit(rebasedHash.get(), censusInstance.namespace(), censusInstance.configuration().census().domain(), - comment.author().id()); + command.user().id()); var issues = checkablePr.createVisitor(localHash, censusInstance); var additionalConfiguration = AdditionalConfiguration.get(localRepo, localHash, pr.repository().forge().currentUser(), allComments); diff --git a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SummaryCommand.java b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SummaryCommand.java index dc49bfce7..3f42aadb9 100644 --- a/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SummaryCommand.java +++ b/bots/pr/src/main/java/org/openjdk/skara/bots/pr/SummaryCommand.java @@ -27,67 +27,37 @@ import java.io.PrintWriter; import java.nio.file.Path; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; +import java.util.*; public class SummaryCommand implements CommandHandler { @Override - public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, String args, Comment comment, List allComments, PrintWriter reply) { - if (!comment.author().equals(pr.author())) { + public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List allComments, PrintWriter reply) { + if (!command.user().equals(pr.author())) { reply.println("Only the author (@" + pr.author().userName() + ") is allowed to issue the `/summary` command."); return; } var currentSummary = Summary.summary(pr.repository().forge().currentUser(), allComments); - var lines = Arrays.asList(comment.body().split("\n")); - - if (args.isBlank()) { - if (lines.size() == 1) { - if (currentSummary.isPresent()) { - reply.println("Removing existing summary"); - reply.println(Summary.setSummaryMarker("")); - } else { - reply.println("To set a summary, use the syntax `/summary `"); - } + if (command.args().isBlank()) { + if (currentSummary.isPresent()) { + reply.println("Removing existing summary"); + reply.println(Summary.setSummaryMarker("")); } else { - // A multi-line summary - var summaryLines = lines.subList(1, lines.size()) - .stream() - .dropWhile(String::isEmpty) - .collect(Collectors.toList()); - var lastIndexWithNonBlankLine = summaryLines.size() - 1; - while (lastIndexWithNonBlankLine >= 0 && summaryLines.get(lastIndexWithNonBlankLine).isEmpty()) { - lastIndexWithNonBlankLine--; - } - if (lastIndexWithNonBlankLine == 0) { - reply.println("To set a summary, use the syntax `/summary `"); - } else { - var summary = String.join("\n", summaryLines.subList(0, lastIndexWithNonBlankLine + 1)); - var action = currentSummary.isPresent() ? "Updating existing" : "Setting"; - reply.println(action + " summary to:\n" + - "\n" + - "```\n" + - summary + - "\n```"); - reply.println(Summary.setSummaryMarker(summary)); - } + reply.println("To set a summary, use the syntax `/summary `"); } } else { - // A single-line summary - if (lines.size() > 1) { - reply.println("To set a multi-line summary, use the syntax:\n" + - "\n```\n" + - "/summary\n" + - "This is the first line\n" + - "This is the second line\n" + - "```"); + var summary = command.args().strip(); + var action = currentSummary.isPresent() ? "Updating existing" : "Setting"; + if (summary.contains("\n")) { + reply.println(action + " summary to:\n" + + "\n" + + "```\n" + + summary + + "\n```"); } else { - var summary = args.strip(); - var action = currentSummary.isPresent() ? "Updating existing" : "Setting"; reply.println(action + " summary to `" + summary + "`"); - reply.println(Summary.setSummaryMarker(summary)); } + reply.println(Summary.setSummaryMarker(summary)); } } @@ -95,4 +65,14 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst public String description() { return "updates the summary in the commit message"; } + + @Override + public boolean multiLine() { + return true; + } + + @Override + public boolean allowedInBody() { + return true; + } } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java index 98b7139e0..a016366e0 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/CommandTests.java @@ -22,13 +22,13 @@ */ package org.openjdk.skara.bots.pr; -import org.openjdk.skara.test.*; - import org.junit.jupiter.api.*; +import org.openjdk.skara.test.*; import java.io.IOException; import static org.junit.jupiter.api.Assertions.*; +import static org.openjdk.skara.bots.pr.PullRequestAsserts.assertLastCommentContains; class CommandTests { @Test @@ -104,6 +104,49 @@ void helpCommand(TestInfo testInfo) throws IOException { } } + @Test + void multipleCommands(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addAuthor(author.forge().currentUser().id()); + var mergeBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "refs/heads/edit", true); + var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request"); + + // Issue multiple commands in a comment + pr.addComment("/contributor add A \n/summary line 1\nline 2\n/contributor add B "); + TestBotRunner.runPeriodicItems(mergeBot); + + // Each command should get a separate reply + assertEquals(4, pr.comments().size()); + assertTrue(pr.comments().get(1).body().contains("Contributor `A ` successfully added"), pr.comments().get(1).body()); + assertTrue(pr.comments().get(2).body().contains("Setting summary to:\n" + + "\n" + + "```\n" + + "line 1\n" + + "line 2"), pr.comments().get(2).body()); + assertTrue(pr.comments().get(3).body().contains("Contributor `B ` successfully added"), pr.comments().get(3).body()); + + // They should only be executed once + TestBotRunner.runPeriodicItems(mergeBot); + TestBotRunner.runPeriodicItems(mergeBot); + assertEquals(4, pr.comments().size()); + } + } + @Test void selfCommand(TestInfo testInfo) throws IOException { try (var credentials = new HostCredentials(testInfo); @@ -149,4 +192,72 @@ void selfCommand(TestInfo testInfo) throws IOException { assertEquals(1, help); } } + + @Test + void inBody(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addAuthor(author.forge().currentUser().id()); + var mergeBot = PullRequestBot.newBuilder() + .repo(integrator) + .censusRepo(censusBuilder.build()) + .build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request"); + + // Issue an invalid body command + pr.setBody("This is a body\n/contributor add A \n/contributor add B "); + TestBotRunner.runPeriodicItems(mergeBot); + + // The second command reply should be the last comment + assertLastCommentContains(pr, "Contributor `B ` successfully added."); + + // The first command should also be reflected in the body + assertTrue(pr.body().contains("A ``")); + } + } + + @Test + void disallowedInBody(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo); + var tempFolder = new TemporaryDirectory()) { + var author = credentials.getHostedRepository(); + var integrator = credentials.getHostedRepository(); + + var censusBuilder = credentials.getCensusBuilder() + .addAuthor(author.forge().currentUser().id()); + var mergeBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build(); + + // Populate the projects repository + var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType()); + var masterHash = localRepo.resolve("master").orElseThrow(); + assertFalse(CheckableRepository.hasBeenEdited(localRepo)); + localRepo.push(masterHash, author.url(), "master", true); + + // Make a change with a corresponding PR + var editHash = CheckableRepository.appendAndCommit(localRepo); + localRepo.push(editHash, author.url(), "edit", true); + var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request"); + + // Issue an invalid body command + pr.setBody("/help"); + TestBotRunner.runPeriodicItems(mergeBot); + + // The bot should reply with some help + assertLastCommentContains(pr, "The command `help` cannot be used in the pull request body"); + } + } } diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java index 985065ff8..172c95d75 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java @@ -183,9 +183,14 @@ void notChecked(TestInfo testInfo) throws IOException { localRepo.push(editHash, author.url(), "refs/heads/edit", true); var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request"); - // Attempt a merge, do not run the check from the bot + // Attempt a merge, but point the check at some other commit pr.addComment("/integrate"); - TestBotRunner.runPeriodicItems(mergeBot, item -> item instanceof CheckWorkItem); + TestBotRunner.runPeriodicItems(mergeBot, item -> { + if (item instanceof CheckWorkItem) { + var newCheck = CheckBuilder.create("jcheck", masterHash).build(); + pr.updateCheck(newCheck); + } + }); // The bot should reply with an error message var error = pr.comments().stream() @@ -305,9 +310,14 @@ void outdatedCheck(TestInfo testInfo) throws IOException { var updatedHash = CheckableRepository.appendAndCommit(localRepo, "Yet another line"); localRepo.push(updatedHash, author.url(), "edit", true); - // Attempt a merge - avoid running checks from the bot + // Attempt a merge, but point the check at some other commit pr.addComment("/integrate"); - TestBotRunner.runPeriodicItems(mergeBot, item -> item instanceof CheckWorkItem); + TestBotRunner.runPeriodicItems(mergeBot, item -> { + if (item instanceof CheckWorkItem) { + var newCheck = CheckBuilder.create("jcheck", masterHash).build(); + pr.updateCheck(newCheck); + } + }); // The bot should reply with an error message var error = pr.comments().stream() @@ -580,15 +590,15 @@ void retryOnFailure(TestInfo testInfo) throws IOException { var badCensusHash = localCensus.commit("Bad census update", "duke", "duke@openjdk.org"); localCensus.push(badCensusHash, censusRepo.url(), "master", true); - // Attempt a merge (without triggering another check) + // Attempt a merge pr.addComment("/integrate"); - assertThrows(RuntimeException.class, () -> TestBotRunner.runPeriodicItems(mergeBot, wi -> wi instanceof CheckWorkItem)); + assertThrows(RuntimeException.class, () -> TestBotRunner.runPeriodicItems(mergeBot)); // Restore the census localCensus.push(currentCensusHash, censusRepo.url(), "master", true); // The bot should now retry - TestBotRunner.runPeriodicItems(mergeBot, wi -> wi instanceof CheckWorkItem); + TestBotRunner.runPeriodicItems(mergeBot); // The bot should reply with an ok message var pushed = pr.comments().stream() diff --git a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SummaryTests.java b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SummaryTests.java index cbbc35d32..b32180976 100644 --- a/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SummaryTests.java +++ b/bots/pr/src/test/java/org/openjdk/skara/bots/pr/SummaryTests.java @@ -376,8 +376,14 @@ void singleAndMultiLineSummary(TestInfo testInfo) throws IOException { pr.addComment("/summary inline\nnext line"); TestBotRunner.runPeriodicItems(prBot); - // The bot should reply with a help message - assertLastCommentContains(pr,"To set a multi-line summary, use the syntax:"); + // This should also be interpreted as a multi-line summary + assertLastCommentContains(pr, + "Setting summary to:\n" + + "\n" + + "```\n" + + "inline\n" + + "next line\n" + + "```"); } } } diff --git a/test/src/main/java/org/openjdk/skara/test/TestBotRunner.java b/test/src/main/java/org/openjdk/skara/test/TestBotRunner.java index a11acd287..f095d80bd 100644 --- a/test/src/main/java/org/openjdk/skara/test/TestBotRunner.java +++ b/test/src/main/java/org/openjdk/skara/test/TestBotRunner.java @@ -26,28 +26,31 @@ import java.io.IOException; import java.util.*; -import java.util.function.Predicate; public class TestBotRunner { + @FunctionalInterface + public interface AfterItemHook { + void run(WorkItem item); + } + public static void runPeriodicItems(Bot bot) throws IOException { - runPeriodicItems(bot, wi -> false); + runPeriodicItems(bot, item -> {}); } - public static void runPeriodicItems(Bot bot, Predicate ignored) throws IOException { + public static void runPeriodicItems(Bot bot, AfterItemHook afterItemHook) throws IOException { var items = new LinkedList<>(bot.getPeriodicItems()); for (var item = items.pollFirst(); item != null; item = items.pollFirst()) { - if (!ignored.test(item)) { - Collection followUpItems = null; - try (var scratchFolder = new TemporaryDirectory()) { - followUpItems = item.run(scratchFolder.path()); - } catch (RuntimeException e) { - item.handleRuntimeException(e); - // Allow tests to assert on these as well - throw e; - } - if (followUpItems != null) { - items.addAll(followUpItems); - } + Collection followUpItems = null; + try (var scratchFolder = new TemporaryDirectory()) { + followUpItems = item.run(scratchFolder.path()); + afterItemHook.run(item); + } catch (RuntimeException e) { + item.handleRuntimeException(e); + // Allow tests to assert on these as well + throw e; + } + if (followUpItems != null) { + items.addAll(followUpItems); } } }