Merge branch 'stable-2.16' into stable-3.0 * stable-2.16: (49 commits) Add change number to task output Only copy required folders for testing Add missing copyright/license header Use a bash associative array to speedup task tests plugins/task: Move TaskExpression iteration to Preloader Add bazel rule for junit tests Introduce TaskKey, SubSectionKey and FileKey plugins/task: Fix cache name in Preloader Only reload nodes when needed Skip re-expanding properties for local properties Use a lazy loading task property expansion model Refresh TaskTree.Nodes when getting them Cache preloaded tasks Rename a bunch of TaskTree addNode() methods Add preload-task to external file tests Add Root Preload tasks-factory test Create a TaskExpression with unit tests Do not modify definition during preload Return Optional<Task> in Task.Config Make task config collection fields immutable ... Change-Id: I4908973e957bba36f81ac2d7c836320faba18b35
diff --git a/.bazelversion b/.bazelversion index fd2a018..7c69a55 100644 --- a/.bazelversion +++ b/.bazelversion
@@ -1 +1 @@ -3.1.0 +3.7.0
diff --git a/BUILD b/BUILD index dbe73f9..39b3d0d 100644 --- a/BUILD +++ b/BUILD
@@ -37,8 +37,6 @@ "Implementation-Title: Task Plugin", "Implementation-URL: https://gerrit-review.googlesource.com/#/admin/projects/plugins/" + plugin_name, "Gerrit-Module: com.googlesource.gerrit.plugins.task.Modules$Module", - "Gerrit-SshModule: com.googlesource.gerrit.plugins.task.Modules$SshModule", - "Gerrit-HttpModule: com.googlesource.gerrit.plugins.task.Modules$HttpModule", ], resource_jars = [":gr-task-plugin-static"], resources = glob(["src/main/resources/**/*"]),
diff --git a/WORKSPACE b/WORKSPACE index b254afa..0c9c56f 100644 --- a/WORKSPACE +++ b/WORKSPACE
@@ -3,7 +3,7 @@ load("//:bazlets.bzl", "load_bazlets") load_bazlets( - commit = "0ccc066431ad7e88a5cd9e06000ce677de1116ee", + commit = "a88174652e6f853ead5bbc5dacc1030dbb2d50c3", #local_path = "/home/<user>/projects/bazlets", ) @@ -51,20 +51,11 @@ sha1 = "eff48ed53995db2dadf0456426cc1f8700136f86", ) -# Release Plugin API +# Load plugin API load( "@com_googlesource_gerrit_bazlets//:gerrit_api.bzl", "gerrit_api", ) -# Snapshot Plugin API -#load( -# "@com_googlesource_gerrit_bazlets//:gerrit_api_maven_local.bzl", -# "gerrit_api_maven_local", -#) - -# Load release Plugin API +# Release Plugin API gerrit_api() - -# Load snapshot Plugin API -#gerrit_api_maven_local()
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/MatchCache.java b/src/main/java/com/googlesource/gerrit/plugins/task/MatchCache.java index 53c41e1..45fe46d 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/MatchCache.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/MatchCache.java
@@ -14,9 +14,9 @@ package com.googlesource.gerrit.plugins.task; +import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.server.query.change.ChangeData; -import com.google.gwtorm.server.OrmException; import java.util.HashMap; import java.util.Map; @@ -31,7 +31,7 @@ this.changeData = changeData; } - protected boolean match(String query) throws OrmException, QueryParseException { + protected boolean match(String query) throws StorageException, QueryParseException { if (query == null) { return true; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/Modules.java b/src/main/java/com/googlesource/gerrit/plugins/task/Modules.java index 6346a73..11786e6 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/Modules.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/Modules.java
@@ -19,7 +19,8 @@ import com.google.gerrit.extensions.webui.JavaScriptPlugin; import com.google.gerrit.extensions.webui.WebUiPlugin; import com.google.gerrit.server.DynamicOptions.DynamicBean; -import com.google.gerrit.server.query.change.ChangeQueryProcessor.ChangeAttributeFactory; +import com.google.gerrit.server.change.ChangeAttributeFactory; +import com.google.gerrit.server.restapi.change.GetChange; import com.google.gerrit.server.restapi.change.QueryChanges; import com.google.gerrit.sshd.commands.Query; import com.google.inject.AbstractModule; @@ -36,19 +37,9 @@ bind(ChangeAttributeFactory.class) .annotatedWith(Exports.named("task")) .to(TaskAttributeFactory.class); - } - } - public static class SshModule extends AbstractModule { - @Override - protected void configure() { + bind(DynamicBean.class).annotatedWith(Exports.named(GetChange.class)).to(MyOptions.class); bind(DynamicBean.class).annotatedWith(Exports.named(Query.class)).to(MyOptions.class); - } - } - - public static class HttpModule extends AbstractModule { - @Override - protected void configure() { bind(DynamicBean.class).annotatedWith(Exports.named(QueryChanges.class)).to(MyOptions.class); DynamicSet.bind(binder(), WebUiPlugin.class) .toInstance(new JavaScriptPlugin("gr-task-plugin.html"));
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java b/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java index aa3da13..7896417 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/PredicateCache.java
@@ -14,12 +14,12 @@ package com.googlesource.gerrit.plugins.task; +import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.index.query.Predicate; import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeQueryBuilder; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import java.util.HashMap; import java.util.Map; @@ -37,7 +37,7 @@ this.cqb = cqb; } - public boolean match(ChangeData c, String query) throws OrmException, QueryParseException { + public boolean match(ChangeData c, String query) throws StorageException, QueryParseException { if (query == null) { return true; } @@ -48,14 +48,14 @@ if (query != null) { try { return matchWithExceptions(c, query); - } catch (OrmException | QueryParseException | RuntimeException e) { + } catch (QueryParseException | RuntimeException e) { } } return null; } protected boolean matchWithExceptions(ChangeData c, String query) - throws QueryParseException, OrmException { + throws QueryParseException, StorageException { if ("true".equalsIgnoreCase(query)) { return true; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java index 57c12ee..5f9a8d5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
@@ -15,9 +15,9 @@ package com.googlesource.gerrit.plugins.task; import com.google.common.collect.Sets; +import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.query.change.ChangeData; -import com.google.gwtorm.server.OrmException; import com.googlesource.gerrit.plugins.task.TaskConfig.NamesFactory; import com.googlesource.gerrit.plugins.task.TaskConfig.Task; import java.lang.reflect.Field; @@ -57,7 +57,7 @@ } /** Use to expand properties specifically for Tasks. */ - public Task getTask(ChangeData changeData) throws OrmException { + public Task getTask(ChangeData changeData) throws StorageException { if (loader != null && loader.isNonTaskDefinedPropertyLoaded()) { // To detect NamesFactories dependent on non task defined properties, the checking must be // done after subnodes are fully loaded, which unfortunately happens after getTask() is @@ -141,14 +141,14 @@ if (changeProp != name) { try { return change(changeProp); - } catch (OrmException e) { + } catch (StorageException e) { throw new RuntimeException(e); } } return ""; } - protected String change(String changeProp) throws OrmException { + protected String change(String changeProp) throws StorageException { switch (changeProp) { case "number": return String.valueOf(change().getId().get()); @@ -171,7 +171,7 @@ if (change == null) { try { change = changeData.change(); - } catch (OrmException e) { + } catch (StorageException e) { throw new RuntimeException(e); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java index 41f87d2..08add9f 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
@@ -15,12 +15,12 @@ package com.googlesource.gerrit.plugins.task; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.index.query.QueryParseException; +import com.google.gerrit.server.DynamicOptions.BeanProvider; +import com.google.gerrit.server.change.ChangeAttributeFactory; import com.google.gerrit.server.query.change.ChangeData; -import com.google.gerrit.server.query.change.ChangeQueryProcessor; -import com.google.gerrit.server.query.change.ChangeQueryProcessor.ChangeAttributeFactory; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.googlesource.gerrit.plugins.task.TaskConfig.Task; import com.googlesource.gerrit.plugins.task.TaskTree.Node; @@ -77,8 +77,8 @@ } @Override - public PluginDefinedInfo create(ChangeData c, ChangeQueryProcessor qp, String plugin) { - options = (Modules.MyOptions) qp.getDynamicBean(plugin); + public PluginDefinedInfo create(ChangeData c, BeanProvider beanProvider, String plugin) { + options = (Modules.MyOptions) beanProvider.getDynamicBean(plugin); if (options.all || options.onlyApplicable || options.onlyInvalid) { for (PatchSetArgument psa : options.patchSetArguments) { definitions.masquerade(psa); @@ -98,7 +98,7 @@ new AttributeFactory(node).create().ifPresent(t -> a.roots.add(t)); } } - } catch (ConfigInvalidException | IOException | OrmException e) { + } catch (ConfigInvalidException | IOException | StorageException e) { a.roots.add(invalid()); } @@ -172,7 +172,6 @@ } } catch (ConfigInvalidException | IOException - | OrmException | QueryParseException | RuntimeException e) { return Optional.of(invalid()); // bad applicability query @@ -180,7 +179,7 @@ return Optional.empty(); } - protected Status getStatusWithExceptions() throws OrmException, QueryParseException { + protected Status getStatusWithExceptions() throws StorageException, QueryParseException { if (isAllNull(task.pass, task.fail, attribute.subTasks)) { // A leaf def has no defined subdefs. boolean hasDefinedSubtasks = @@ -244,13 +243,13 @@ protected Status getStatus() { try { return getStatusWithExceptions(); - } catch (OrmException | QueryParseException | RuntimeException e) { + } catch (QueryParseException | RuntimeException e) { return Status.INVALID; } } protected List<TaskAttribute> getSubTasks() - throws ConfigInvalidException, IOException, OrmException { + throws ConfigInvalidException, IOException, StorageException { List<TaskAttribute> subTasks = new ArrayList<>(); for (Node subNode : node.getSubNodes()) { if (subNode == null) { @@ -275,7 +274,7 @@ matchCache.match(task.fail); matchCache.match(task.pass); return true; - } catch (OrmException | QueryParseException | RuntimeException e) { + } catch (QueryParseException | RuntimeException e) { return false; } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java index 5746c10..2bd8cf6 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -15,6 +15,8 @@ package com.googlesource.gerrit.plugins.task; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.exceptions.StorageException; +import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; @@ -26,7 +28,6 @@ import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeQueryBuilder; import com.google.gerrit.server.query.change.ChangeQueryProcessor; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.googlesource.gerrit.plugins.task.TaskConfig.External; @@ -101,7 +102,7 @@ } public List<Node> getRootNodes(ChangeData changeData) - throws ConfigInvalidException, IOException, OrmException { + throws ConfigInvalidException, IOException, StorageException { this.changeData = changeData; root.path = Collections.emptyList(); return root.getSubNodes(); @@ -114,22 +115,22 @@ protected List<Node> nodes; protected Set<String> names = new HashSet<>(); - protected void addSubNodes() throws ConfigInvalidException, IOException, OrmException { + protected void addSubNodes() throws ConfigInvalidException, IOException, StorageException { addPreloaded(preloader.getRootTasks(taskFactory.getRootConfig())); } - protected void addPreloaded(List<Task> defs) throws ConfigInvalidException, OrmException { + protected void addPreloaded(List<Task> defs) throws ConfigInvalidException, StorageException { for (Task def : defs) { addPreloaded(def); } } - protected void addPreloaded(Task def) throws ConfigInvalidException, OrmException { + protected void addPreloaded(Task def) throws ConfigInvalidException, StorageException { addPreloaded(def, (parent, definition) -> new Node(parent, definition)); } protected void addPreloaded(Task def, NodeFactory nodeFactory) - throws ConfigInvalidException, OrmException { + throws ConfigInvalidException, StorageException { if (def != null) { try { Node node = cachedNodeByTask.get(def.key()); @@ -157,7 +158,7 @@ nodes.add(null); // null node indicates invalid } - protected List<Node> getSubNodes() throws ConfigInvalidException, IOException, OrmException { + protected List<Node> getSubNodes() throws ConfigInvalidException, IOException, StorageException { if (nodes == null) { nodes = new ArrayList<>(); addSubNodes(); @@ -167,7 +168,7 @@ return nodes; } - public void refreshSubNodes() throws ConfigInvalidException, OrmException { + public void refreshSubNodes() throws ConfigInvalidException, StorageException { if (nodes != null) { for (Node node : nodes) { if (node != null) { @@ -192,7 +193,7 @@ protected final Properties properties; protected final TaskKey taskKey; - public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException { + public Node(NodeList parent, Task task) throws ConfigInvalidException, StorageException { this.parent = parent; taskKey = task.key(); properties = new Properties(task, parent.getProperties()); @@ -206,7 +207,7 @@ /* The task needs to be refreshed before a node is used, however subNode refreshing can wait until they are fetched since they may not be needed. */ - public void refreshTask() throws ConfigInvalidException, OrmException { + public void refreshTask() throws ConfigInvalidException, StorageException { this.path = new LinkedList<>(parent.path); this.path.add(key()); @@ -221,14 +222,14 @@ } @Override - protected void addSubNodes() throws ConfigInvalidException, OrmException { + protected void addSubNodes() throws ConfigInvalidException, StorageException { addSubTasks(); addSubTasksFactoryTasks(); addSubTasksFiles(); addSubTasksExternals(); } - protected void addSubTasks() throws ConfigInvalidException, OrmException { + protected void addSubTasks() throws ConfigInvalidException, StorageException { for (String expression : task.subTasks) { try { Optional<Task> def = @@ -242,7 +243,7 @@ } } - protected void addSubTasksFiles() throws ConfigInvalidException, OrmException { + protected void addSubTasksFiles() throws ConfigInvalidException, StorageException { for (String file : task.subTasksFiles) { try { addPreloaded( @@ -253,7 +254,7 @@ } } - protected void addSubTasksExternals() throws ConfigInvalidException, OrmException { + protected void addSubTasksExternals() throws ConfigInvalidException, StorageException { for (String external : task.subTasksExternals) { try { External ext = task.config.getExternal(external); @@ -268,7 +269,7 @@ } } - protected void addSubTasksFactoryTasks() throws ConfigInvalidException, OrmException { + protected void addSubTasksFactoryTasks() throws ConfigInvalidException, StorageException { for (String tasksFactoryName : task.subTasksFactories) { TasksFactory tasksFactory = task.config.getTasksFactory(tasksFactoryName); if (tasksFactory != null) { @@ -290,14 +291,14 @@ } protected void addStaticTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) - throws ConfigInvalidException, OrmException { + throws ConfigInvalidException, StorageException { for (String name : namesFactory.names) { addPreloaded(preloader.preload(task.config.new Task(tasksFactory, name))); } } protected void addChangeTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) - throws ConfigInvalidException, OrmException { + throws ConfigInvalidException, StorageException { try { if (namesFactory.changes != null) { List<ChangeData> changeDataList = @@ -324,7 +325,7 @@ } return; } - } catch (OrmException e) { + } catch (StorageException e) { log.atSevere().withCause(e).log("ERROR: running changes query: " + namesFactory.changes); } catch (QueryParseException e) { } @@ -332,7 +333,7 @@ } protected List<Task> getPreloadedTasks(External external) - throws ConfigInvalidException, IOException, OrmException { + throws ConfigInvalidException, IOException, StorageException { return getPreloadedTasks( FileKey.create(resolveUserBranch(external.user), resolveTaskFileName(external.file))); } @@ -364,14 +365,16 @@ } protected Branch.NameKey resolveUserBranch(String user) - throws ConfigInvalidException, IOException, OrmException { + throws ConfigInvalidException, IOException, StorageException { if (user == null) { throw new ConfigInvalidException("External user not defined"); } - Account acct = accountResolver.find(user); - if (acct == null) { + Account.Id acct; + try { + acct = accountResolver.resolve(user).asUnique().getAccount().getId(); + } catch (UnprocessableEntityException e) { throw new ConfigInvalidException("Cannot resolve user: " + user); } - return new Branch.NameKey(allUsers.get(), RefNames.refsUsers(acct.getId())); + return new Branch.NameKey(allUsers.get(), RefNames.refsUsers(acct)); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/cli/PatchSetArgument.java b/src/main/java/com/googlesource/gerrit/plugins/task/cli/PatchSetArgument.java index bcbb972..9fbddeb 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/cli/PatchSetArgument.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/cli/PatchSetArgument.java
@@ -13,10 +13,10 @@ // limitations under the License. package com.googlesource.gerrit.plugins.task.cli; +import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.notedb.ChangeNotes; @@ -24,14 +24,12 @@ import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.sshd.BaseCommand.UnloggedFailure; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; public class PatchSetArgument { public static class Factory { protected final PermissionBackend permissionBackend; protected final ChangeNotes.Factory notesFactory; - protected final ReviewDb reviewDb; protected final PatchSetUtil psUtil; protected final CurrentUser user; @@ -39,12 +37,10 @@ protected Factory( ChangeNotes.Factory notesFactory, PermissionBackend permissionBackend, - ReviewDb reviewDb, PatchSetUtil psUtil, CurrentUser user) { this.notesFactory = notesFactory; this.permissionBackend = permissionBackend; - this.reviewDb = reviewDb; this.psUtil = psUtil; this.user = user; } @@ -53,18 +49,13 @@ try { PatchSet.Id patchSetId = parsePatchSet(token); ChangeNotes changeNotes = notesFactory.createChecked(patchSetId.getParentKey()); - permissionBackend - .user(user) - .database(reviewDb) - .change(changeNotes) - .check(ChangePermission.READ); - return new PatchSetArgument( - changeNotes.getChange(), psUtil.get(reviewDb, changeNotes, patchSetId)); + permissionBackend.user(user).change(changeNotes).check(ChangePermission.READ); + return new PatchSetArgument(changeNotes.getChange(), psUtil.get(changeNotes, patchSetId)); } catch (PermissionBackendException | AuthException e) { throw new IllegalArgumentException("database error", e); } catch (UnloggedFailure e) { throw new IllegalArgumentException(e.getMessage(), e); - } catch (OrmException e) { + } catch (StorageException e) { throw new IllegalArgumentException("database error", e); } }
diff --git a/test/check_task_statuses.sh b/test/check_task_statuses.sh index a28a15d..1d3efbd 100755 --- a/test/check_task_statuses.sh +++ b/test/check_task_statuses.sh
@@ -223,7 +223,7 @@ } get_change_num() { # < gerrit_push_response > changenum - local url=$(awk '/New Changes:/ { getline; print $2 }') + local url=$(awk '$NF ~ /\[NEW\]/ { print $2 }') echo "${url##*\/}" | tr -d -c '[:digit:]' }
diff --git a/test/docker/gerrit/Dockerfile b/test/docker/gerrit/Dockerfile index 059f3c0..b9d1715 100755 --- a/test/docker/gerrit/Dockerfile +++ b/test/docker/gerrit/Dockerfile
@@ -1,4 +1,4 @@ -FROM gerritcodereview/gerrit:2.16.27-ubuntu16 +FROM gerritcodereview/gerrit:3.0.16-ubuntu18 USER root