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

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) {
+        } catch (RestApiException e) {
-          logger.atSevere().withCause(e).log("Can't update the superprojects");
+          logger.atWarning().withCause(e).log("Can't update the superprojects");
         }
       }
     }

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) {

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);
+          String.format("cannot update gitlink for the commit at branch %s", args.destBranch), e);
-    } catch (SubmoduleException 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);
     }
   }
 }

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.
  *
- * <p>Message should be considered user-visible.
+ * <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 SubmoduleException extends Exception {
+public class SubmoduleConflictException extends ResourceConflictException {
   private static final long serialVersionUID = 1L;
 
-  SubmoduleException(String msg) {
+  public SubmoduleConflictException(String msg) {
-    super(msg, null);
+  super(msg);
-  }
-
-  SubmoduleException(String msg, Throwable why) {
-    super(msg, why);
   }
 }

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<BranchNameKey> 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<BranchNameKey> updatedBranches,
       MergeOpRepoManager orm)
-      throws SubmoduleException {
+      throws SubmoduleConflictException {
     this.gitmodulesFactory = gitmodulesFactory;
     this.myIdent = myIdent;
     this.projectCache = projectCache;
@@ -199,7 +200,6 @@ public class SubmoduleOp {
    * </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 @@ public class SubmoduleOp {
   //
   // 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 @@ public class SubmoduleOp {
       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 @@ 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<Project.NameKey> 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) {
+    } catch (UpdateException | IOException | NoSuchProjectException e) {
-      throw new SubmoduleException("Cannot update gitlinks", 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)
+      RevCommit oldCommit) {
-      throws SubmoduleException {
     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<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 @@ public class SubmoduleOp {
       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));
     }