Submodule updates: Differentiate between conflicts and server errors If a submodule operation cannot be performed we currently always throw a SubmoduleException. According to its javadoc SubmoduleException messages are user-visible. SubmoduleException is thrown in 2 cases: 1. the submodule operation cannot be performed due to conflicts 2. the submodule operation cannot be performed due to an error in Gerrit For 1. we should return '409 Conflict’, for 2. rather ‘500 Internal Server Error'. In case of 2. the exception message should not be returned to users. To fix this we introduce a SubmoduleConflictException as subclass of ResourceConflictException that is thrown if there is a conflict when performing the submodule operation and the user should get a '409 Conflict' response. In case of internal server errors we throw StorageException now. Since StorageException is a RuntimeException this cleans up our method signatures a bit. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: Icd60fa14a67944543ccf06df38ffee03c5d0c722
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 2ce7446..263d06c 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -163,7 +163,6 @@ import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.submit.MergeOp; import com.google.gerrit.server.submit.MergeOpRepoManager; -import com.google.gerrit.server.submit.SubmoduleException; import com.google.gerrit.server.submit.SubmoduleOp; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; @@ -763,8 +762,8 @@ orm.setContext(TimeUtil.nowTs(), user, NotifyResolver.Result.none()); SubmoduleOp op = subOpFactory.create(branches, orm); op.updateSuperProjects(); - } catch (SubmoduleException e) { - logger.atSevere().withCause(e).log("Can't update the superprojects"); + } catch (RestApiException e) { + logger.atWarning().withCause(e).log("Can't update the superprojects"); } } }
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 800fe0e..dc24a9d 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -622,7 +622,7 @@ throw new ResourceNotFoundException(e.getMessage()); } catch (IOException e) { throw new StorageException(e); - } catch (SubmoduleException e) { + } catch (SubmoduleConflictException e) { throw new IntegrationConflictException(e.getMessage(), e); } catch (UpdateException e) { if (e.getCause() instanceof LockFailureException) {
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java index 1625528..a4141be 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -552,10 +552,13 @@ return args.submoduleOp.composeGitlinksCommit(args.destBranch, commit); } catch (IOException e) { throw new StorageException( - "cannot update gitlink for the commit at branch: " + args.destBranch, e); - } catch (SubmoduleException e) { + String.format("cannot update gitlink for the commit at branch %s", args.destBranch), e); + } catch (SubmoduleConflictException e) { throw new IntegrationConflictException( - "cannot update gitlink for the commit at branch: " + args.destBranch, e); + String.format( + "cannot update gitlink for the commit at branch %s: %s", + args.destBranch, e.getMessage()), + e); } } }
diff --git a/java/com/google/gerrit/server/submit/SubmoduleConflictException.java b/java/com/google/gerrit/server/submit/SubmoduleConflictException.java new file mode 100644 index 0000000..23f583e --- /dev/null +++ b/java/com/google/gerrit/server/submit/SubmoduleConflictException.java
@@ -0,0 +1,31 @@ +// 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.extensions.restapi.ResourceConflictException; + +/** + * Exception to be thrown if any submodule operation is not possible due to conflicts. + * + * <p>Throwing this exception results in a {@code 409 Conflict} response to the calling user. The + * exception message is returned as error message to the user. + */ +public class SubmoduleConflictException extends ResourceConflictException { + private static final long serialVersionUID = 1L; + + public SubmoduleConflictException(String msg) { + super(msg); + } +}
diff --git a/java/com/google/gerrit/server/submit/SubmoduleException.java b/java/com/google/gerrit/server/submit/SubmoduleException.java deleted file mode 100644 index 2367d0a..0000000 --- a/java/com/google/gerrit/server/submit/SubmoduleException.java +++ /dev/null
@@ -1,32 +0,0 @@ -// Copyright (C) 2011 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; - -/** - * Indicates the gitlink's update cannot be processed at this time. - * - * <p>Message should be considered user-visible. - */ -public class SubmoduleException extends Exception { - private static final long serialVersionUID = 1L; - - SubmoduleException(String msg) { - super(msg, null); - } - - SubmoduleException(String msg, Throwable why) { - super(msg, why); - } -}
diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 9c0bc77..b48076194 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java
@@ -28,6 +28,7 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; 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; @@ -116,7 +117,7 @@ } public SubmoduleOp create(Set<BranchNameKey> updatedBranches, MergeOpRepoManager orm) - throws SubmoduleException { + throws SubmoduleConflictException { return new SubmoduleOp( gitmodulesFactory, serverIdent.get(), cfg, projectCache, updatedBranches, orm); } @@ -166,7 +167,7 @@ ProjectCache projectCache, Set<BranchNameKey> updatedBranches, MergeOpRepoManager orm) - throws SubmoduleException { + throws SubmoduleConflictException { this.gitmodulesFactory = gitmodulesFactory; this.myIdent = myIdent; this.projectCache = projectCache; @@ -199,7 +200,6 @@ * </ul> * * @return the ordered set to be stored in {@link #sortedBranches}. - * @throws SubmoduleException if an error occurred walking projects. */ // TODO(dborowitz): This setup process is hard to follow, in large part due to the accumulation of // mutable maps, which makes this whole class difficult to understand. @@ -215,7 +215,8 @@ // // In addition to improving readability, this approach has the advantage of making (1) and (2) // testable using small tests. - private ImmutableSet<BranchNameKey> calculateSubscriptionMaps() throws SubmoduleException { + private ImmutableSet<BranchNameKey> calculateSubscriptionMaps() + throws SubmoduleConflictException { if (!enableSuperProjectSubscriptions) { logger.atFine().log("Updating superprojects disabled"); return null; @@ -244,11 +245,11 @@ BranchNameKey current, LinkedHashSet<BranchNameKey> currentVisited, LinkedHashSet<BranchNameKey> allVisited) - throws SubmoduleException { + throws SubmoduleConflictException { logger.atFine().log("Now processing %s", current); if (currentVisited.contains(current)) { - throw new SubmoduleException( + throw new SubmoduleConflictException( "Branch level circular subscriptions detected: " + printCircularPath(currentVisited, current)); } @@ -270,7 +271,7 @@ affectedBranches.add(sub.getSubmodule()); } } catch (IOException e) { - throw new SubmoduleException("Cannot find superprojects for " + current, e); + throw new StorageException("Cannot find superprojects for " + current, e); } currentVisited.remove(current); allVisited.add(current); @@ -396,7 +397,7 @@ return ret; } - public void updateSuperProjects() throws SubmoduleException { + public void updateSuperProjects() throws RestApiException { ImmutableSet<Project.NameKey> projects = getProjectsInOrder(); if (projects == null) { return; @@ -416,19 +417,19 @@ } } BatchUpdate.execute(orm.batchUpdates(superProjects), BatchUpdateListener.NONE, false); - } catch (RestApiException | UpdateException | IOException | NoSuchProjectException e) { - throw new SubmoduleException("Cannot update gitlinks", e); + } catch (UpdateException | IOException | NoSuchProjectException e) { + throw new StorageException("Cannot update gitlinks", e); } } /** Create a separate gitlink commit */ private CodeReviewCommit composeGitlinksCommit(BranchNameKey subscriber) - throws IOException, SubmoduleException { + throws IOException, SubmoduleConflictException { OpenRepo or; try { or = orm.getRepo(subscriber.project()); } catch (NoSuchProjectException | IOException e) { - throw new SubmoduleException("Cannot access superproject", e); + throw new StorageException("Cannot access superproject", e); } CodeReviewCommit currentCommit; @@ -437,7 +438,7 @@ } else { Ref r = or.repo.exactRef(subscriber.branch()); if (r == null) { - throw new SubmoduleException( + throw new SubmoduleConflictException( "The branch was probably deleted from the subscriber repository"); } currentCommit = or.rw.parseCommit(r.getObjectId()); @@ -493,12 +494,12 @@ /** Amend an existing commit with gitlink updates */ CodeReviewCommit composeGitlinksCommit(BranchNameKey subscriber, CodeReviewCommit currentCommit) - throws IOException, SubmoduleException { + throws IOException, SubmoduleConflictException { OpenRepo or; try { or = orm.getRepo(subscriber.project()); } catch (NoSuchProjectException | IOException e) { - throw new SubmoduleException("Cannot access superproject", e); + throw new StorageException("Cannot access superproject", e); } StringBuilder msgbuf = new StringBuilder(); @@ -534,13 +535,13 @@ private RevCommit updateSubmodule( DirCache dc, DirCacheEditor ed, StringBuilder msgbuf, SubmoduleSubscription s) - throws SubmoduleException, IOException { + throws SubmoduleConflictException, IOException { logger.atFine().log("Updating gitlink for %s", s); OpenRepo subOr; try { subOr = orm.getRepo(s.getSubmodule().project()); } catch (NoSuchProjectException | IOException e) { - throw new SubmoduleException("Cannot access submodule", e); + throw new StorageException("Cannot access submodule", e); } DirCacheEntry dce = dc.getEntry(s.getPath()); @@ -554,7 +555,7 @@ + s.getSubmodule().project().get() + " but entry " + "doesn't have gitlink file mode."; - throw new SubmoduleException(errMsg); + throw new SubmoduleConflictException(errMsg); } // Parse the current gitlink entry commit in the subproject repo. This is used to add a // shortlog for this submodule to the commit message in the superproject. @@ -617,8 +618,7 @@ SubmoduleSubscription s, OpenRepo subOr, RevCommit newCommit, - RevCommit oldCommit) - throws SubmoduleException { + RevCommit oldCommit) { msgbuf.append("* Update "); msgbuf.append(s.getPath()); msgbuf.append(" from branch '"); @@ -659,7 +659,7 @@ msgbuf.append(message); } } catch (IOException e) { - throw new SubmoduleException( + throw new StorageException( "Could not perform a revwalk to create superproject commit message", e); } } @@ -676,7 +676,7 @@ return dc; } - ImmutableSet<Project.NameKey> getProjectsInOrder() throws SubmoduleException { + ImmutableSet<Project.NameKey> getProjectsInOrder() throws SubmoduleConflictException { LinkedHashSet<Project.NameKey> projects = new LinkedHashSet<>(); for (Project.NameKey project : branchesByProject.keySet()) { addAllSubmoduleProjects(project, new LinkedHashSet<>(), projects); @@ -692,9 +692,9 @@ Project.NameKey project, LinkedHashSet<Project.NameKey> current, LinkedHashSet<Project.NameKey> projects) - throws SubmoduleException { + throws SubmoduleConflictException { if (current.contains(project)) { - throw new SubmoduleException( + throw new SubmoduleConflictException( "Project level circular subscriptions detected: " + printCircularPath(current, project)); }
diff --git a/plugins/delete-project b/plugins/delete-project index da0811e..e8a249a 160000 --- a/plugins/delete-project +++ b/plugins/delete-project
@@ -1 +1 @@ -Subproject commit da0811ef34ecdb4cb7183beec60085aff9acc3db +Subproject commit e8a249ae1b68f43d5941f0c1a64ccdbd0a395d4a