From 8fd9e97270375cce089d9d77ff887d190460e152 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Wed, 26 Aug 2020 17:44:35 -0700 Subject: [PATCH 1/6] GitlinkOp: Move it to its own class and factory The submoduleOp is not needed to instantiate GitlinkOps. Make this explicit moving GitlinkOp to its own class with a factory. When SubmoduleCommits and SubscriptionGraph become injectable, this factory can also be injected where needed. Change-Id: Iecf4e8064dd3c2e60a2799a411fbcde5d2805e0d --- .../gerrit/server/submit/GitlinkOp.java | 64 +++++++++++++++++++ .../google/gerrit/server/submit/MergeOp.java | 12 +++- .../gerrit/server/submit/SubmoduleOp.java | 39 +---------- 3 files changed, 76 insertions(+), 39 deletions(-) create mode 100644 java/com/google/gerrit/server/submit/GitlinkOp.java diff --git a/java/com/google/gerrit/server/submit/GitlinkOp.java b/java/com/google/gerrit/server/submit/GitlinkOp.java new file mode 100644 index 0000000000..70a52b6cec --- /dev/null +++ b/java/com/google/gerrit/server/submit/GitlinkOp.java @@ -0,0 +1,64 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.submit; + +import com.google.gerrit.entities.BranchNameKey; +import com.google.gerrit.entities.SubmoduleSubscription; +import com.google.gerrit.server.git.CodeReviewCommit; +import com.google.gerrit.server.update.RepoContext; +import com.google.gerrit.server.update.RepoOnlyOp; +import java.util.Collection; +import java.util.Optional; + +/** Only used for branches without code review changes */ +public class GitlinkOp implements RepoOnlyOp { + + static class Factory { + private SubmoduleCommits submoduleCommits; + private SubscriptionGraph subscriptionGraph; + + Factory(SubmoduleCommits submoduleCommits, SubscriptionGraph subscriptionGraph) { + this.submoduleCommits = submoduleCommits; + this.subscriptionGraph = subscriptionGraph; + } + + GitlinkOp create(BranchNameKey branch) { + return new GitlinkOp(branch, submoduleCommits, subscriptionGraph.getSubscriptions(branch)); + } + } + + private final BranchNameKey branch; + private final SubmoduleCommits commitHelper; + private final Collection branchTargets; + + GitlinkOp( + BranchNameKey branch, + SubmoduleCommits commitHelper, + Collection branchTargets) { + this.branch = branch; + this.commitHelper = commitHelper; + this.branchTargets = branchTargets; + } + + @Override + public void updateRepo(RepoContext ctx) throws Exception { + Optional commit = commitHelper.composeGitlinksCommit(branch, branchTargets); + if (commit.isPresent()) { + CodeReviewCommit c = commit.get(); + ctx.addRefUpdate(c.getParent(0), c, branch.branch()); + commitHelper.addBranchTip(branch, c); + } + } +} diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index a0c1b8267b..e46786880e 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -664,6 +664,12 @@ public class MergeOp implements AutoCloseable { Set allBranches = submoduleOp.getBranchesInOrder(); Set allCommits = toSubmit.values().stream().map(BranchBatch::commits).flatMap(Set::stream).collect(toSet()); + + // TODO(ifrade): injection for these classes + SubmoduleCommits submoduleCommits = submoduleOp.getSubmoduleCommits(); + SubscriptionGraph subscriptionGraph = submoduleOp.getSubscriptionGraph(); + GitlinkOp.Factory gitlinkOpFactory = new GitlinkOp.Factory(submoduleCommits, subscriptionGraph); + for (BranchNameKey branch : allBranches) { OpenRepo or = orm.getRepo(branch.project()); if (toSubmit.containsKey(branch)) { @@ -694,13 +700,13 @@ public class MergeOp implements AutoCloseable { strategies.add(strategy); strategy.addOps(or.getUpdate(), commitsToSubmit); if (submitting.submitType().equals(SubmitType.FAST_FORWARD_ONLY) - && submoduleOp.getSubscriptionGraph().hasSubscription(branch)) { - submoduleOp.addOp(or.getUpdate(), branch); + && subscriptionGraph.hasSubscription(branch)) { + or.getUpdate().addRepoOnlyOp(gitlinkOpFactory.create(branch)); } } else { // no open change for this branch // add submodule triggered op into BatchUpdate - submoduleOp.addOp(or.getUpdate(), branch); + or.getUpdate().addRepoOnlyOp(gitlinkOpFactory.create(branch)); } } return strategies; diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 3cea62bd7d..89f40660c9 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java @@ -24,13 +24,10 @@ import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.config.GerritServerConfig; -import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateListener; -import com.google.gerrit.server.update.RepoContext; -import com.google.gerrit.server.update.RepoOnlyOp; import com.google.gerrit.server.update.UpdateException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -39,7 +36,6 @@ import java.io.IOException; import java.util.Collection; import java.util.HashSet; import java.util.LinkedHashSet; -import java.util.Optional; import java.util.Set; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; @@ -47,32 +43,6 @@ import org.eclipse.jgit.lib.PersonIdent; public class SubmoduleOp { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - /** Only used for branches without code review changes */ - public static class GitlinkOp implements RepoOnlyOp { - private final BranchNameKey branch; - private final SubmoduleCommits commitHelper; - private final Collection branchTargets; - - GitlinkOp( - BranchNameKey branch, - SubmoduleCommits commitHelper, - Collection branchTargets) { - this.branch = branch; - this.commitHelper = commitHelper; - this.branchTargets = branchTargets; - } - - @Override - public void updateRepo(RepoContext ctx) throws Exception { - Optional commit = commitHelper.composeGitlinksCommit(branch, branchTargets); - if (commit.isPresent()) { - CodeReviewCommit c = commit.get(); - ctx.addRefUpdate(c.getParent(0), c, branch.branch()); - commitHelper.addBranchTip(branch, c); - } - } - } - @Singleton public static class Factory { private final SubscriptionGraph.Factory subscriptionGraphFactory; @@ -140,6 +110,8 @@ public class SubmoduleOp { LinkedHashSet superProjects = new LinkedHashSet<>(); try { + GitlinkOp.Factory gitlinkOpFactory = + new GitlinkOp.Factory(submoduleCommits, subscriptionGraph); for (Project.NameKey project : projects) { // only need superprojects if (subscriptionGraph.isAffectedSuperProject(project)) { @@ -147,7 +119,7 @@ public class SubmoduleOp { // get a new BatchUpdate for the super project OpenRepo or = orm.getRepo(project); for (BranchNameKey branch : subscriptionGraph.getAffectedSuperBranches(project)) { - addOp(or.getUpdate(), branch); + or.getUpdate().addRepoOnlyOp(gitlinkOpFactory.create(branch)); } } } @@ -207,9 +179,4 @@ public class SubmoduleOp { branches.addAll(subscriptionGraph.getUpdatedBranches()); return ImmutableSet.copyOf(branches); } - - void addOp(BatchUpdate bu, BranchNameKey branch) { - bu.addRepoOnlyOp( - new GitlinkOp(branch, submoduleCommits, subscriptionGraph.getSubscriptions(branch))); - } } From 55e3f9ce4ab64b4d8c028bbd80dca75804d66af3 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Wed, 16 Sep 2020 16:17:19 -0700 Subject: [PATCH 2/6] SubmoduleCommits: Define its own guice factory SubmoduleOp is acting only as an instantiator of SubmoduleCommits. Give SubmoduleCommits its own Guice factory and use it in submoduleOp. Eventually callers could move from submoduleOp.getSubmoduleCommits() to use directly the factory. Change-Id: Ib320b0ed966336f6add9548f31fcf6a6feb727c0 --- .../server/submit/SubmoduleCommits.java | 21 +++++++++++++++++++ .../gerrit/server/submit/SubmoduleOp.java | 18 +++++++--------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/java/com/google/gerrit/server/submit/SubmoduleCommits.java b/java/com/google/gerrit/server/submit/SubmoduleCommits.java index 419ce0118b..1312a4bb33 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleCommits.java +++ b/java/com/google/gerrit/server/submit/SubmoduleCommits.java @@ -21,10 +21,15 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.SubmoduleSubscription; import com.google.gerrit.exceptions.StorageException; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.VerboseSuperprojectUpdate; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; import java.io.IOException; import java.util.Collection; import java.util.Iterator; @@ -58,6 +63,22 @@ class SubmoduleCommits { private final long maxCommitMessages; private final BranchTips branchTips = new BranchTips(); + @Singleton + public static class Factory { + private final Provider serverIdent; + private final Config cfg; + + @Inject + Factory(@GerritPersonIdent Provider serverIdent, @GerritServerConfig Config cfg) { + this.serverIdent = serverIdent; + this.cfg = cfg; + } + + public SubmoduleCommits create(MergeOpRepoManager orm) { + return new SubmoduleCommits(orm, serverIdent.get(), cfg); + } + } + SubmoduleCommits(MergeOpRepoManager orm, PersonIdent myIdent, Config cfg) { this.orm = orm; this.myIdent = myIdent; diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 89f40660c9..7cec6c1f20 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java @@ -22,7 +22,6 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.entities.SubmoduleSubscription; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo; @@ -30,7 +29,6 @@ import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateListener; import com.google.gerrit.server.update.UpdateException; import com.google.inject.Inject; -import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.util.Collection; @@ -38,7 +36,6 @@ import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.PersonIdent; public class SubmoduleOp { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @@ -46,16 +43,16 @@ public class SubmoduleOp { @Singleton public static class Factory { private final SubscriptionGraph.Factory subscriptionGraphFactory; - private final Provider serverIdent; private final Config cfg; + private final SubmoduleCommits.Factory submoduleCommitsFactory; @Inject Factory( SubscriptionGraph.Factory subscriptionGraphFactory, - @GerritPersonIdent Provider serverIdent, + SubmoduleCommits.Factory submoduleCommitsFactory, @GerritServerConfig Config cfg) { this.subscriptionGraphFactory = subscriptionGraphFactory; - this.serverIdent = serverIdent; + this.submoduleCommitsFactory = submoduleCommitsFactory; this.cfg = cfg; } @@ -69,7 +66,7 @@ public class SubmoduleOp { subscriptionGraph = SubscriptionGraph.createEmptyGraph(ImmutableSet.copyOf(updatedBranches)); } - return new SubmoduleOp(serverIdent.get(), cfg, orm, subscriptionGraph); + return new SubmoduleOp(orm, subscriptionGraph, submoduleCommitsFactory.create(orm)); } } @@ -78,13 +75,12 @@ public class SubmoduleOp { private final SubmoduleCommits submoduleCommits; private SubmoduleOp( - PersonIdent myIdent, - Config cfg, MergeOpRepoManager orm, - SubscriptionGraph subscriptionGraph) { + SubscriptionGraph subscriptionGraph, + SubmoduleCommits submoduleCommits) { this.orm = orm; this.subscriptionGraph = subscriptionGraph; - this.submoduleCommits = new SubmoduleCommits(orm, myIdent, cfg); + this.submoduleCommits = submoduleCommits; } // TODO(ifrade): subscription graph should be instantiated somewhere else and passed to From f2a511895a15e80bc4274a2a521eab76a62f0ac3 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Thu, 17 Sep 2020 14:18:08 -0700 Subject: [PATCH 3/6] UpdateOrderCalculator: Move update order calculation out of submoduleOp This cryptic code calculates the precedence of updates, so all subprojects are updated before the superprojects. It is in submoduleOp, but actually it only depends on the subscription graph. Note that the subscription graph is immutable for the request and therefore this class too. All this becomes more visible with the code in its own class. Change-Id: I66ed2bd8520718a5023d64935db410ee16bf220d --- .../google/gerrit/server/submit/MergeOp.java | 4 +- .../gerrit/server/submit/SubmoduleOp.java | 62 ++----------- .../server/submit/UpdateOrderCalculator.java | 91 +++++++++++++++++++ 3 files changed, 100 insertions(+), 57 deletions(-) create mode 100644 java/com/google/gerrit/server/submit/UpdateOrderCalculator.java diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index e46786880e..a0cba212e1 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -607,7 +607,7 @@ public class MergeOp implements AutoCloseable { try { SubmoduleOp submoduleOp = subOpFactory.create(branches, orm); List strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun); - this.allProjects = submoduleOp.getProjectsInOrder(); + this.allProjects = submoduleOp.getUpdateOrderCalculator().getProjectsInOrder(); try { BatchUpdate.execute( orm.batchUpdates(allProjects), @@ -661,7 +661,7 @@ public class MergeOp implements AutoCloseable { Map toSubmit, SubmoduleOp submoduleOp, boolean dryrun) throws IntegrationConflictException, NoSuchProjectException, IOException { List strategies = new ArrayList<>(); - Set allBranches = submoduleOp.getBranchesInOrder(); + Set allBranches = submoduleOp.getUpdateOrderCalculator().getBranchesInOrder(); Set allCommits = toSubmit.values().stream().map(BranchBatch::commits).flatMap(Set::stream).collect(toSet()); diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 7cec6c1f20..0ee7572a72 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java @@ -19,7 +19,6 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.UsedAt; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Project; -import com.google.gerrit.entities.SubmoduleSubscription; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.config.GerritServerConfig; @@ -31,8 +30,6 @@ import com.google.gerrit.server.update.UpdateException; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; -import java.util.Collection; -import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; import org.eclipse.jgit.lib.Config; @@ -73,6 +70,7 @@ public class SubmoduleOp { private final MergeOpRepoManager orm; private final SubscriptionGraph subscriptionGraph; private final SubmoduleCommits submoduleCommits; + private final UpdateOrderCalculator updateOrderCalculator; private SubmoduleOp( MergeOpRepoManager orm, @@ -81,6 +79,7 @@ public class SubmoduleOp { this.orm = orm; this.subscriptionGraph = subscriptionGraph; this.submoduleCommits = submoduleCommits; + this.updateOrderCalculator = new UpdateOrderCalculator(subscriptionGraph); } // TODO(ifrade): subscription graph should be instantiated somewhere else and passed to @@ -93,13 +92,17 @@ public class SubmoduleOp { return submoduleCommits; } + UpdateOrderCalculator getUpdateOrderCalculator() { + return updateOrderCalculator; + } + @UsedAt(UsedAt.Project.PLUGIN_DELETE_PROJECT) public boolean hasSuperproject(BranchNameKey branch) { return subscriptionGraph.hasSuperproject(branch); } public void updateSuperProjects() throws RestApiException { - ImmutableSet projects = getProjectsInOrder(); + ImmutableSet projects = updateOrderCalculator.getProjectsInOrder(); if (projects == null) { return; } @@ -124,55 +127,4 @@ public class SubmoduleOp { throw new StorageException("Cannot update gitlinks", e); } } - - ImmutableSet getProjectsInOrder() throws SubmoduleConflictException { - LinkedHashSet projects = new LinkedHashSet<>(); - for (Project.NameKey project : subscriptionGraph.getAffectedSuperProjects()) { - addAllSubmoduleProjects(project, new LinkedHashSet<>(), projects); - } - - for (BranchNameKey branch : subscriptionGraph.getUpdatedBranches()) { - projects.add(branch.project()); - } - return ImmutableSet.copyOf(projects); - } - - private void addAllSubmoduleProjects( - Project.NameKey project, - LinkedHashSet current, - LinkedHashSet projects) - throws SubmoduleConflictException { - if (current.contains(project)) { - throw new SubmoduleConflictException( - "Project level circular subscriptions detected: " - + CircularPathFinder.printCircularPath(current, project)); - } - - if (projects.contains(project)) { - return; - } - - current.add(project); - Set subprojects = new HashSet<>(); - for (BranchNameKey branch : subscriptionGraph.getAffectedSuperBranches(project)) { - Collection subscriptions = subscriptionGraph.getSubscriptions(branch); - for (SubmoduleSubscription s : subscriptions) { - subprojects.add(s.getSubmodule().project()); - } - } - - for (Project.NameKey p : subprojects) { - addAllSubmoduleProjects(p, current, projects); - } - - current.remove(project); - projects.add(project); - } - - ImmutableSet getBranchesInOrder() { - LinkedHashSet branches = new LinkedHashSet<>(); - branches.addAll(subscriptionGraph.getSortedSuperprojectAndSubmoduleBranches()); - branches.addAll(subscriptionGraph.getUpdatedBranches()); - return ImmutableSet.copyOf(branches); - } } diff --git a/java/com/google/gerrit/server/submit/UpdateOrderCalculator.java b/java/com/google/gerrit/server/submit/UpdateOrderCalculator.java new file mode 100644 index 0000000000..517c708e09 --- /dev/null +++ b/java/com/google/gerrit/server/submit/UpdateOrderCalculator.java @@ -0,0 +1,91 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.submit; + +import com.google.common.collect.ImmutableSet; +import com.google.gerrit.entities.BranchNameKey; +import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.SubmoduleSubscription; +import java.util.Collection; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.Set; + +/** + * Sorts the projects or branches affected by the update. + * + *

The subscription graph contains all branches (and projects) affected by the update, but the + * updates must be executed in the right order, so no superproject reference is updated before its + * target. + */ +class UpdateOrderCalculator { + + private final SubscriptionGraph subscriptionGraph; + + UpdateOrderCalculator(SubscriptionGraph subscriptionGraph) { + this.subscriptionGraph = subscriptionGraph; + } + + ImmutableSet getProjectsInOrder() throws SubmoduleConflictException { + LinkedHashSet projects = new LinkedHashSet<>(); + for (Project.NameKey project : subscriptionGraph.getAffectedSuperProjects()) { + addAllSubmoduleProjects(project, new LinkedHashSet<>(), projects); + } + + for (BranchNameKey branch : subscriptionGraph.getUpdatedBranches()) { + projects.add(branch.project()); + } + return ImmutableSet.copyOf(projects); + } + + private void addAllSubmoduleProjects( + Project.NameKey project, + LinkedHashSet current, + LinkedHashSet projects) + throws SubmoduleConflictException { + if (current.contains(project)) { + throw new SubmoduleConflictException( + "Project level circular subscriptions detected: " + + CircularPathFinder.printCircularPath(current, project)); + } + + if (projects.contains(project)) { + return; + } + + current.add(project); + Set subprojects = new HashSet<>(); + for (BranchNameKey branch : subscriptionGraph.getAffectedSuperBranches(project)) { + Collection subscriptions = subscriptionGraph.getSubscriptions(branch); + for (SubmoduleSubscription s : subscriptions) { + subprojects.add(s.getSubmodule().project()); + } + } + + for (Project.NameKey p : subprojects) { + addAllSubmoduleProjects(p, current, projects); + } + + current.remove(project); + projects.add(project); + } + + ImmutableSet getBranchesInOrder() { + LinkedHashSet branches = new LinkedHashSet<>(); + branches.addAll(subscriptionGraph.getSortedSuperprojectAndSubmoduleBranches()); + branches.addAll(subscriptionGraph.getUpdatedBranches()); + return ImmutableSet.copyOf(branches); + } +} From 8c05f6380930484601984d8ee72813bafdfcff0c Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Thu, 17 Sep 2020 15:52:30 -0700 Subject: [PATCH 4/6] SubmoduleOp: Move subscription graph instantiation to its own factory Subscription graph has a factory, but there is an extra layer of indirection to take config into account. This indirection is inside submoduleOp. SubmoduleOp needs the subscription graph but shouldn't control its instantiation. Move this to an extra Factory, so clients can instantiate their subscription graph without depending on submoduleOp. In guice, bind the unnannotated Subscription.Factory to the configured factory and annotate the "vanilla" version. Change-Id: Ie1ab3bfaebbe0480a5e4f93bbb5796b5e6a8e740 --- .../server/config/GerritGlobalModule.java | 3 ++ .../ConfiguredSubscriptionGraphFactory.java | 54 +++++++++++++++++++ .../gerrit/server/submit/SubmoduleOp.java | 22 ++------ .../server/submit/SubscriptionGraph.java | 2 +- .../submit/VanillaSubscriptionGraph.java | 29 ++++++++++ 5 files changed, 92 insertions(+), 18 deletions(-) create mode 100644 java/com/google/gerrit/server/submit/ConfiguredSubscriptionGraphFactory.java create mode 100644 java/com/google/gerrit/server/submit/VanillaSubscriptionGraph.java diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 0023395841..22d02d2401 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -187,9 +187,11 @@ import com.google.gerrit.server.rules.PrologModule; import com.google.gerrit.server.rules.RulesCache; import com.google.gerrit.server.rules.SubmitRule; import com.google.gerrit.server.ssh.SshAddressesModule; +import com.google.gerrit.server.submit.ConfiguredSubscriptionGraphFactory; import com.google.gerrit.server.submit.GitModules; import com.google.gerrit.server.submit.MergeSuperSetComputation; import com.google.gerrit.server.submit.SubmitStrategy; +import com.google.gerrit.server.submit.SubscriptionGraph; import com.google.gerrit.server.tools.ToolsCatalog; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.util.IdGenerator; @@ -453,6 +455,7 @@ public class GerritGlobalModule extends FactoryModule { factory(VersionedAuthorizedKeys.Factory.class); bind(AccountManager.class); + bind(SubscriptionGraph.Factory.class).to(ConfiguredSubscriptionGraphFactory.class); bind(new TypeLiteral>() {}).toProvider(CommentLinkProvider.class); DynamicSet.bind(binder(), GerritConfigListener.class).to(CommentLinkProvider.class); diff --git a/java/com/google/gerrit/server/submit/ConfiguredSubscriptionGraphFactory.java b/java/com/google/gerrit/server/submit/ConfiguredSubscriptionGraphFactory.java new file mode 100644 index 0000000000..d9ef03bf69 --- /dev/null +++ b/java/com/google/gerrit/server/submit/ConfiguredSubscriptionGraphFactory.java @@ -0,0 +1,54 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.submit; + +import com.google.common.collect.ImmutableSet; +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.entities.BranchNameKey; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.Inject; +import java.util.Set; +import org.eclipse.jgit.lib.Config; + +/** + * Wrap a (@link {@link SubscriptionGraph.Factory} to honor the gerrit configuration. + * + *

If superproject subscriptions are disabled in the conf, return an empty graph. + */ +public class ConfiguredSubscriptionGraphFactory implements SubscriptionGraph.Factory { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private final SubscriptionGraph.Factory subscriptionGraphFactory; + private final Config cfg; + + @Inject + ConfiguredSubscriptionGraphFactory( + @VanillaSubscriptionGraph SubscriptionGraph.Factory subscriptionGraphFactory, + @GerritServerConfig Config cfg) { + this.subscriptionGraphFactory = subscriptionGraphFactory; + this.cfg = cfg; + } + + @Override + public SubscriptionGraph compute(Set updatedBranches, MergeOpRepoManager orm) + throws SubmoduleConflictException { + if (cfg.getBoolean("submodule", "enableSuperProjectSubscriptions", true)) { + return subscriptionGraphFactory.compute(updatedBranches, orm); + } else { + logger.atFine().log("Updating superprojects disabled"); + return SubscriptionGraph.createEmptyGraph(ImmutableSet.copyOf(updatedBranches)); + } + } +} diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 0ee7572a72..94a432a5d9 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java @@ -15,13 +15,11 @@ package com.google.gerrit.server.submit; import com.google.common.collect.ImmutableSet; -import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.UsedAt; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo; import com.google.gerrit.server.update.BatchUpdate; @@ -32,38 +30,28 @@ import com.google.inject.Singleton; import java.io.IOException; import java.util.LinkedHashSet; import java.util.Set; -import org.eclipse.jgit.lib.Config; public class SubmoduleOp { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @Singleton public static class Factory { private final SubscriptionGraph.Factory subscriptionGraphFactory; - private final Config cfg; private final SubmoduleCommits.Factory submoduleCommitsFactory; @Inject Factory( SubscriptionGraph.Factory subscriptionGraphFactory, - SubmoduleCommits.Factory submoduleCommitsFactory, - @GerritServerConfig Config cfg) { + SubmoduleCommits.Factory submoduleCommitsFactory) { this.subscriptionGraphFactory = subscriptionGraphFactory; this.submoduleCommitsFactory = submoduleCommitsFactory; - this.cfg = cfg; } public SubmoduleOp create(Set updatedBranches, MergeOpRepoManager orm) throws SubmoduleConflictException { - SubscriptionGraph subscriptionGraph; - if (cfg.getBoolean("submodule", "enableSuperProjectSubscriptions", true)) { - subscriptionGraph = subscriptionGraphFactory.compute(updatedBranches, orm); - } else { - logger.atFine().log("Updating superprojects disabled"); - subscriptionGraph = - SubscriptionGraph.createEmptyGraph(ImmutableSet.copyOf(updatedBranches)); - } - return new SubmoduleOp(orm, subscriptionGraph, submoduleCommitsFactory.create(orm)); + return new SubmoduleOp( + orm, + subscriptionGraphFactory.compute(updatedBranches, orm), + submoduleCommitsFactory.create(orm)); } } diff --git a/java/com/google/gerrit/server/submit/SubscriptionGraph.java b/java/com/google/gerrit/server/submit/SubscriptionGraph.java index b08e46d1ff..b11b2bec5a 100644 --- a/java/com/google/gerrit/server/submit/SubscriptionGraph.java +++ b/java/com/google/gerrit/server/submit/SubscriptionGraph.java @@ -159,7 +159,7 @@ public class SubscriptionGraph { public static class Module extends AbstractModule { @Override protected void configure() { - bind(Factory.class).to(DefaultFactory.class); + bind(Factory.class).annotatedWith(VanillaSubscriptionGraph.class).to(DefaultFactory.class); } } diff --git a/java/com/google/gerrit/server/submit/VanillaSubscriptionGraph.java b/java/com/google/gerrit/server/submit/VanillaSubscriptionGraph.java new file mode 100644 index 0000000000..a88157eb10 --- /dev/null +++ b/java/com/google/gerrit/server/submit/VanillaSubscriptionGraph.java @@ -0,0 +1,29 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.submit; + +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import com.google.inject.BindingAnnotation; +import java.lang.annotation.Retention; + +/** + * Marker on a {@link SubscriptionGraph.Factory} without gerrit configuration. + * + *

See {@link ConfiguredSubscriptionGraphFactory}. + */ +@Retention(RUNTIME) +@BindingAnnotation +public @interface VanillaSubscriptionGraph {} From 0551d5baa1f7807c5605952de9280d418170fa26 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Thu, 17 Sep 2020 14:39:49 -0700 Subject: [PATCH 5/6] MergeOp: Pass the specific objects, instead of submoduleOp SubmoduleOp is just acting as factory of all these components. To unravel that, first make the code use directly the components. Change-Id: Ib623ce65187bb23686d1af33a92fdbf32d153ae7 --- .../google/gerrit/server/submit/MergeOp.java | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index a0cba212e1..354bd491a9 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -606,8 +606,13 @@ public class MergeOp implements AutoCloseable { try { SubmoduleOp submoduleOp = subOpFactory.create(branches, orm); - List strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun); - this.allProjects = submoduleOp.getUpdateOrderCalculator().getProjectsInOrder(); + UpdateOrderCalculator updateOrderCalculator = submoduleOp.getUpdateOrderCalculator(); + SubscriptionGraph subscriptionGraph = submoduleOp.getSubscriptionGraph(); + SubmoduleCommits submoduleCommits = submoduleOp.getSubmoduleCommits(); + List strategies = + getSubmitStrategies( + toSubmit, updateOrderCalculator, submoduleCommits, subscriptionGraph, dryrun); + this.allProjects = updateOrderCalculator.getProjectsInOrder(); try { BatchUpdate.execute( orm.batchUpdates(allProjects), @@ -658,16 +663,17 @@ public class MergeOp implements AutoCloseable { } private List getSubmitStrategies( - Map toSubmit, SubmoduleOp submoduleOp, boolean dryrun) + Map toSubmit, + UpdateOrderCalculator updateOrderCalculator, + SubmoduleCommits submoduleCommits, + SubscriptionGraph subscriptionGraph, + boolean dryrun) throws IntegrationConflictException, NoSuchProjectException, IOException { List strategies = new ArrayList<>(); - Set allBranches = submoduleOp.getUpdateOrderCalculator().getBranchesInOrder(); + Set allBranches = updateOrderCalculator.getBranchesInOrder(); Set allCommits = toSubmit.values().stream().map(BranchBatch::commits).flatMap(Set::stream).collect(toSet()); - // TODO(ifrade): injection for these classes - SubmoduleCommits submoduleCommits = submoduleOp.getSubmoduleCommits(); - SubscriptionGraph subscriptionGraph = submoduleOp.getSubscriptionGraph(); GitlinkOp.Factory gitlinkOpFactory = new GitlinkOp.Factory(submoduleCommits, subscriptionGraph); for (BranchNameKey branch : allBranches) { @@ -694,8 +700,8 @@ public class MergeOp implements AutoCloseable { commitStatus, submissionId, submitInput, - submoduleOp.getSubmoduleCommits(), - submoduleOp.getSubscriptionGraph(), + submoduleCommits, + subscriptionGraph, dryrun); strategies.add(strategy); strategy.addOps(or.getUpdate(), commitsToSubmit); From 32dcbb9951fb4128de2a5b21e46ddabe3d38d7df Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Thu, 17 Sep 2020 15:24:23 -0700 Subject: [PATCH 6/6] MergeOp: Use factories of the subscription components After this, submoduleOp is not the instantiator of all this classes and can focus on what it does: updating superprojects. Change-Id: Ie5d1b547fc3815356591c4ccefa9944ec0867559 --- java/com/google/gerrit/server/submit/MergeOp.java | 13 +++++++++---- .../google/gerrit/server/submit/SubmoduleOp.java | 14 -------------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 354bd491a9..b304dd0004 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -226,7 +226,9 @@ public class MergeOp implements AutoCloseable { private final MergeValidators.Factory mergeValidatorsFactory; private final Provider queryProvider; private final SubmitStrategyFactory submitStrategyFactory; + private final SubscriptionGraph.Factory subscriptionGraphFactory; private final SubmoduleOp.Factory subOpFactory; + private final SubmoduleCommits.Factory submoduleCommitsFactory; private final Provider ormProvider; private final NotifyResolver notifyResolver; private final RetryHelper retryHelper; @@ -256,6 +258,8 @@ public class MergeOp implements AutoCloseable { MergeValidators.Factory mergeValidatorsFactory, Provider queryProvider, SubmitStrategyFactory submitStrategyFactory, + SubmoduleCommits.Factory submoduleCommitsFactory, + SubscriptionGraph.Factory subscriptionGraphFactory, SubmoduleOp.Factory subOpFactory, Provider ormProvider, NotifyResolver notifyResolver, @@ -269,6 +273,8 @@ public class MergeOp implements AutoCloseable { this.mergeValidatorsFactory = mergeValidatorsFactory; this.queryProvider = queryProvider; this.submitStrategyFactory = submitStrategyFactory; + this.submoduleCommitsFactory = submoduleCommitsFactory; + this.subscriptionGraphFactory = subscriptionGraphFactory; this.subOpFactory = subOpFactory; this.ormProvider = ormProvider; this.notifyResolver = notifyResolver; @@ -605,10 +611,9 @@ public class MergeOp implements AutoCloseable { commitStatus.maybeFailVerbose(); try { - SubmoduleOp submoduleOp = subOpFactory.create(branches, orm); - UpdateOrderCalculator updateOrderCalculator = submoduleOp.getUpdateOrderCalculator(); - SubscriptionGraph subscriptionGraph = submoduleOp.getSubscriptionGraph(); - SubmoduleCommits submoduleCommits = submoduleOp.getSubmoduleCommits(); + SubscriptionGraph subscriptionGraph = subscriptionGraphFactory.compute(branches, orm); + SubmoduleCommits submoduleCommits = submoduleCommitsFactory.create(orm); + UpdateOrderCalculator updateOrderCalculator = new UpdateOrderCalculator(subscriptionGraph); List strategies = getSubmitStrategies( toSubmit, updateOrderCalculator, submoduleCommits, subscriptionGraph, dryrun); diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 94a432a5d9..8bfcd51d23 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java @@ -70,20 +70,6 @@ public class SubmoduleOp { this.updateOrderCalculator = new UpdateOrderCalculator(subscriptionGraph); } - // TODO(ifrade): subscription graph should be instantiated somewhere else and passed to - // SubmoduleOp - SubscriptionGraph getSubscriptionGraph() { - return subscriptionGraph; - } - - SubmoduleCommits getSubmoduleCommits() { - return submoduleCommits; - } - - UpdateOrderCalculator getUpdateOrderCalculator() { - return updateOrderCalculator; - } - @UsedAt(UsedAt.Project.PLUGIN_DELETE_PROJECT) public boolean hasSuperproject(BranchNameKey branch) { return subscriptionGraph.hasSuperproject(branch);