From 2cf3dd9c750e14c69da26126d6e3dab5fd28bb01 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 9 Mar 2020 09:13:56 +0100 Subject: [PATCH] Submodule updates: Differentiate between conflicts and server errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Change-Id: Icd60fa14a67944543ccf06df38ffee03c5d0c722 --- .../server/git/receive/ReceiveCommits.java | 5 +- .../google/gerrit/server/submit/MergeOp.java | 2 +- .../server/submit/SubmitStrategyOp.java | 9 ++-- ...n.java => SubmoduleConflictException.java} | 19 ++++---- .../gerrit/server/submit/SubmoduleOp.java | 48 +++++++++---------- plugins/delete-project | 2 +- 6 files changed, 43 insertions(+), 42 deletions(-) rename java/com/google/gerrit/server/submit/{SubmoduleException.java => SubmoduleConflictException.java} (56%) diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 2ce7446d0f..263d06c85a 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.ChangeData; 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 @@ class ReceiveCommits { 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 800fe0e143..dc24a9d877 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -622,7 +622,7 @@ public class MergeOp implements AutoCloseable { 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 16255280a8..a4141be119 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java @@ -552,10 +552,13 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { 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/SubmoduleException.java b/java/com/google/gerrit/server/submit/SubmoduleConflictException.java similarity index 56% rename from java/com/google/gerrit/server/submit/SubmoduleException.java rename to java/com/google/gerrit/server/submit/SubmoduleConflictException.java index 2367d0a520..23f583e9cd 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleException.java +++ b/java/com/google/gerrit/server/submit/SubmoduleConflictException.java @@ -1,4 +1,4 @@ -// Copyright (C) 2011 The Android Open Source Project +// 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. @@ -14,19 +14,18 @@ package com.google.gerrit.server.submit; +import com.google.gerrit.extensions.restapi.ResourceConflictException; + /** - * Indicates the gitlink's update cannot be processed at this time. + * Exception to be thrown if any submodule operation is not possible due to conflicts. * - *

Message should be considered user-visible. + *

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 SubmoduleException extends Exception { +public class SubmoduleConflictException extends ResourceConflictException { private static final long serialVersionUID = 1L; - SubmoduleException(String msg) { - super(msg, null); - } - - SubmoduleException(String msg, Throwable why) { - super(msg, why); + public SubmoduleConflictException(String msg) { + super(msg); } } diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 9c0bc77507..b48076194c 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.BranchNameKey; 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 class SubmoduleOp { } public SubmoduleOp create(Set updatedBranches, MergeOpRepoManager orm) - throws SubmoduleException { + throws SubmoduleConflictException { return new SubmoduleOp( gitmodulesFactory, serverIdent.get(), cfg, projectCache, updatedBranches, orm); } @@ -166,7 +167,7 @@ public class SubmoduleOp { ProjectCache projectCache, Set updatedBranches, MergeOpRepoManager orm) - throws SubmoduleException { + throws SubmoduleConflictException { this.gitmodulesFactory = gitmodulesFactory; this.myIdent = myIdent; this.projectCache = projectCache; @@ -199,7 +200,6 @@ public class SubmoduleOp { * * * @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 @@ public class SubmoduleOp { // // In addition to improving readability, this approach has the advantage of making (1) and (2) // testable using small tests. - private ImmutableSet calculateSubscriptionMaps() throws SubmoduleException { + private ImmutableSet calculateSubscriptionMaps() + throws SubmoduleConflictException { if (!enableSuperProjectSubscriptions) { logger.atFine().log("Updating superprojects disabled"); return null; @@ -244,11 +245,11 @@ public class SubmoduleOp { BranchNameKey current, LinkedHashSet currentVisited, LinkedHashSet 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 @@ public class SubmoduleOp { 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 @@ public class SubmoduleOp { return ret; } - public void updateSuperProjects() throws SubmoduleException { + public void updateSuperProjects() throws RestApiException { ImmutableSet projects = getProjectsInOrder(); if (projects == null) { return; @@ -416,19 +417,19 @@ public class SubmoduleOp { } } 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 @@ public class SubmoduleOp { } 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 @@ public class SubmoduleOp { /** 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 @@ public class SubmoduleOp { 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 @@ public class SubmoduleOp { + 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 @@ public class SubmoduleOp { 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 @@ public class SubmoduleOp { 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 @@ public class SubmoduleOp { return dc; } - ImmutableSet getProjectsInOrder() throws SubmoduleException { + ImmutableSet getProjectsInOrder() throws SubmoduleConflictException { LinkedHashSet projects = new LinkedHashSet<>(); for (Project.NameKey project : branchesByProject.keySet()) { addAllSubmoduleProjects(project, new LinkedHashSet<>(), projects); @@ -692,9 +692,9 @@ public class SubmoduleOp { Project.NameKey project, LinkedHashSet current, LinkedHashSet 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 da0811ef34..e8a249ae1b 160000 --- a/plugins/delete-project +++ b/plugins/delete-project @@ -1 +1 @@ -Subproject commit da0811ef34ecdb4cb7183beec60085aff9acc3db +Subproject commit e8a249ae1b68f43d5941f0c1a64ccdbd0a395d4a