Make StorageException a RuntimeException

In the context of a Gerrit server, StorageExceptions are generally
speaking not recoverable: the only place that should be catching them is
a top-level request handler such as RestApiServlet. Such a handler also
needs to be able to catch RuntimeExceptions, and indeed RestApiServlet
already catches bare Exception.

The general best practice in Java today is to use unchecked exception
when errors are not intended to be recoverable[citation needed]. This
avoids polluting interfaces with too many checked exception types.

After this change, most "throws StorageException" clauses are redundant
and can be cleaned up. However, that work is left for future commits;
this commit does the minimum work required to compile.

This commit does not additionally touch DuplicateKeyException, which
also becomes a RuntimeException since it inherits from StorageException.

Change-Id: I75230cf99145d886a5d872150e333519eb54acbb

java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java

@@ -18,7 +18,6 @@ import static com.google.inject.Scopes.SINGLETON;
 
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.exceptions.StorageRuntimeException;
 import com.google.gerrit.extensions.events.LifecycleListener;
 import com.google.gerrit.lifecycle.LifecycleModule;
 import com.google.gerrit.metrics.DisabledMetricMaker;
@@ -91,8 +90,8 @@ class InMemoryTestingDatabaseModule extends LifecycleModule {
     public void start() {
       try {
         schemaCreator.ensureCreated();
-      } catch (StorageException | IOException | ConfigInvalidException e) {
-        throw new StorageRuntimeException(e);
+      } catch (IOException | ConfigInvalidException e) {
+        throw new StorageException(e);
       }
     }
 

java/com/google/gerrit/exceptions/StorageException.java

@@ -27,7 +27,7 @@ package com.google.gerrit.exceptions;
  *   <li>Other wrapped {@code IOException}s
  * </ul>
  */
-public class StorageException extends Exception {
+public class StorageException extends RuntimeException {
   private static final long serialVersionUID = 1L;
 
   public StorageException(String message) {

java/com/google/gerrit/exceptions/StorageRuntimeException.java

@@ -1,32 +0,0 @@
-// Copyright 2008 Google Inc.
-//
-// 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.exceptions;
-
-/** Any data store read or write error. */
-public class StorageRuntimeException extends RuntimeException {
-  private static final long serialVersionUID = 1L;
-
-  public StorageRuntimeException(String message) {
-    super(message);
-  }
-
-  public StorageRuntimeException(String message, Throwable why) {
-    super(message, why);
-  }
-
-  public StorageRuntimeException(Throwable why) {
-    super(why);
-  }
-}

java/com/google/gerrit/index/query/AndSource.java

@@ -17,12 +17,10 @@ package com.google.gerrit.index.query;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
-import com.google.common.base.Throwables;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.exceptions.StorageRuntimeException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Comparator;
@@ -77,23 +75,6 @@ public class AndSource<T> extends AndPredicate<T>
 
   @Override
   public ResultSet<T> read() throws StorageException {
-    try {
-      return readImpl();
-    } catch (StorageRuntimeException err) {
-      if (err.getCause() != null) {
-        Throwables.throwIfInstanceOf(err.getCause(), StorageException.class);
-      }
-      throw new StorageException(err);
-    }
-  }
-
-  @Override
-  public ResultSet<FieldBundle> readRaw() throws StorageException {
-    // TOOD(hiesel): Implement
-    throw new UnsupportedOperationException("not implemented");
-  }
-
-  private ResultSet<T> readImpl() throws StorageException {
     if (source == null) {
       throw new StorageException("No DataSource: " + this);
     }
@@ -141,6 +122,12 @@ public class AndSource<T> extends AndPredicate<T>
     return new ListResultSet<>(r);
   }
 
+  @Override
+  public ResultSet<FieldBundle> readRaw() throws StorageException {
+    // TOOD(hiesel): Implement
+    throw new UnsupportedOperationException("not implemented");
+  }
+
   @Override
   public boolean isMatchable() {
     return isVisibleToPredicate != null || super.isMatchable();
@@ -164,7 +151,7 @@ public class AndSource<T> extends AndPredicate<T>
         .transformAndConcat(this::transformBuffer);
   }
 
-  protected List<T> transformBuffer(List<T> buffer) throws StorageRuntimeException {
+  protected List<T> transformBuffer(List<T> buffer) {
     return buffer;
   }
 

java/com/google/gerrit/index/query/QueryProcessor.java

@@ -27,7 +27,6 @@ import com.google.common.collect.Ordering;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.exceptions.StorageRuntimeException;
 import com.google.gerrit.index.Index;
 import com.google.gerrit.index.IndexCollection;
 import com.google.gerrit.index.IndexConfig;
@@ -192,8 +191,6 @@ public abstract class QueryProcessor<T> {
       throws StorageException, QueryParseException {
     try {
       return query(null, queries);
-    } catch (StorageRuntimeException e) {
-      throw new StorageException(e.getMessage(), e);
     } catch (StorageException e) {
       if (e.getCause() != null) {
         Throwables.throwIfInstanceOf(e.getCause(), QueryParseException.class);
@@ -287,7 +284,7 @@ public abstract class QueryProcessor<T> {
       // Only measure successful queries that actually touched the index.
       metrics.executionTime.record(
           schemaDef.getName(), System.nanoTime() - startNanos, TimeUnit.NANOSECONDS);
-    } catch (StorageException | StorageRuntimeException e) {
+    } catch (StorageException e) {
       Optional<QueryParseException> qpe = findQueryParseException(e);
       if (qpe.isPresent()) {
         throw new QueryParseException(qpe.get().getMessage(), e);

java/com/google/gerrit/lucene/LuceneChangeIndex.java

@@ -37,7 +37,6 @@ import com.google.common.flogger.FluentLogger;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.exceptions.StorageRuntimeException;
 import com.google.gerrit.index.QueryOptions;
 import com.google.gerrit.index.Schema;
 import com.google.gerrit.index.query.FieldBundle;
@@ -415,10 +414,10 @@ public class LuceneChangeIndex implements ChangeIndex {
         return result.build();
       } catch (InterruptedException e) {
         close();
-        throw new StorageRuntimeException(e);
+        throw new StorageException(e);
       } catch (ExecutionException e) {
         Throwables.throwIfUnchecked(e.getCause());
-        throw new StorageRuntimeException(e.getCause());
+        throw new StorageException(e.getCause());
       }
     }
 

java/com/google/gerrit/server/change/ChangeJson.java

@@ -379,7 +379,6 @@ public class ChangeJson {
       return toChangeInfo(cd, limitToPsId, changeInfoSupplier);
     } catch (PatchListNotAvailableException
         | GpgException
-        | StorageException
         | IOException
         | PermissionBackendException
         | RuntimeException e) {
@@ -426,7 +425,7 @@ public class ChangeJson {
         try {
           ensureLoaded(Collections.singleton(cd));
           changeInfos.add(format(cd, Optional.empty(), false, ChangeInfo::new));
-        } catch (StorageException | RuntimeException e) {
+        } catch (RuntimeException e) {
           logger.atWarning().withCause(e).log(
               "Omitting corrupt change %s from results", cd.getId());
         }

java/com/google/gerrit/server/query/change/AndChangeSource.java

@@ -14,8 +14,6 @@
 
 package com.google.gerrit.server.query.change;
 
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.exceptions.StorageRuntimeException;
 import com.google.gerrit.index.query.AndSource;
 import com.google.gerrit.index.query.IsVisibleToPredicate;
 import com.google.gerrit.index.query.Predicate;
@@ -43,14 +41,9 @@ public class AndChangeSource extends AndSource<ChangeData> implements ChangeData
   }
 
   @Override
-  protected List<ChangeData> transformBuffer(List<ChangeData> buffer)
-      throws StorageRuntimeException {
+  protected List<ChangeData> transformBuffer(List<ChangeData> buffer) {
     if (!hasChange()) {
-      try {
-        ChangeData.ensureChangeLoaded(buffer);
-      } catch (StorageException e) {
-        throw new StorageRuntimeException(e);
-      }
+      ChangeData.ensureChangeLoaded(buffer);
     }
     return super.transformBuffer(buffer);
   }

java/com/google/gerrit/server/restapi/change/GetRevisionActions.java

@@ -17,7 +17,6 @@ package com.google.gerrit.server.restapi.change;
 import com.google.common.hash.Hasher;
 import com.google.common.hash.Hashing;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.exceptions.StorageRuntimeException;
 import com.google.gerrit.extensions.common.ActionInfo;
 import com.google.gerrit.extensions.restapi.ETagView;
 import com.google.gerrit.extensions.restapi.Response;
@@ -73,8 +72,8 @@ public class GetRevisionActions implements ETagView<RevisionResource> {
         changeResourceFactory.create(cd.notes(), user).prepareETag(h, user);
       }
       h.putBoolean(cs.furtherHiddenChanges());
-    } catch (IOException | StorageException | PermissionBackendException e) {
-      throw new StorageRuntimeException(e);
+    } catch (IOException | PermissionBackendException e) {
+      throw new StorageException(e);
     }
     return h.hash().toString();
   }

java/com/google/gerrit/server/restapi/change/PreviewSubmit.java

@@ -124,8 +124,7 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
           .setContentType(f.getMimeType())
           .setAttachmentName("submit-preview-" + change.getChangeId() + "." + format);
       return bin;
-    } catch (StorageException
-        | RestApiException
+    } catch (RestApiException
         | UpdateException
         | IOException
         | ConfigInvalidException

java/com/google/gerrit/server/restapi/change/Submit.java

@@ -24,7 +24,6 @@ import com.google.common.collect.Sets;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.data.ParameterizedString;
 import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.exceptions.StorageRuntimeException;
 import com.google.gerrit.extensions.api.changes.SubmitInput;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -69,7 +68,6 @@ import java.util.Collection;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -290,9 +288,9 @@ public class Submit
         return "Problems with change(s): "
             + unmergeable.stream().map(c -> c.getId().toString()).collect(joining(", "));
       }
-    } catch (PermissionBackendException | StorageException | IOException e) {
+    } catch (PermissionBackendException | IOException e) {
       logger.atSevere().withCause(e).log("Error checking if change is submittable");
-      throw new StorageRuntimeException("Could not determine problems for the change", e);
+      throw new StorageException("Could not determine problems for the change", e);
     }
     return null;
   }
@@ -313,7 +311,7 @@ public class Submit
       }
     } catch (IOException e) {
       logger.atSevere().withCause(e).log("Error checking if change is submittable");
-      throw new StorageRuntimeException("Could not determine problems for the change", e);
+      throw new StorageException("Could not determine problems for the change", e);
     }
 
     ChangeData cd = changeDataFactory.create(resource.getNotes());
@@ -321,42 +319,31 @@ public class Submit
       MergeOp.checkSubmitRule(cd, false);
     } catch (ResourceConflictException e) {
       return null; // submit not visible
-    } catch (StorageException e) {
-      logger.atSevere().withCause(e).log("Error checking if change is submittable");
-      throw new StorageRuntimeException("Could not determine problems for the change", e);
     }
 
     ChangeSet cs;
     try {
       cs = mergeSuperSet.get().completeChangeSet(cd.change(), resource.getUser());
-    } catch (StorageException | IOException | PermissionBackendException e) {
-      throw new StorageRuntimeException(
-          "Could not determine complete set of changes to be submitted", e);
+    } catch (IOException | PermissionBackendException e) {
+      throw new StorageException("Could not determine complete set of changes to be submitted", e);
     }
 
     String topic = change.getTopic();
     int topicSize = 0;
     if (!Strings.isNullOrEmpty(topic)) {
-      topicSize = getChangesByTopic(topic).size();
+      topicSize = queryProvider.get().byTopicOpen(topic).size();
     }
     boolean treatWithTopic = submitWholeTopic && !Strings.isNullOrEmpty(topic) && topicSize > 1;
 
     String submitProblems = problemsForSubmittingChangeset(cd, cs, resource.getUser());
 
-    Boolean enabled;
-    try {
-      // Recheck mergeability rather than using value stored in the index,
-      // which may be stale.
-      // TODO(dborowitz): This is ugly; consider providing a way to not read
-      // stored fields from the index in the first place.
-      // cd.setMergeable(null);
-      // That was done in unmergeableChanges which was called by
-      // problemsForSubmittingChangeset, so now it is safe to read from
-      // the cache, as it yields the same result.
-      enabled = cd.isMergeable();
-    } catch (StorageException e) {
-      throw new StorageRuntimeException("Could not determine mergeability", e);
-    }
+    // Recheck mergeability rather than using value stored in the index, which may be stale.
+    // TODO(dborowitz): This is ugly; consider providing a way to not read stored fields from the
+    // index in the first place.
+    // cd.setMergeable(null);
+    // That was done in unmergeableChanges which was called by problemsForSubmittingChangeset, so
+    // now it is safe to read from the cache, as it yields the same result.
+    Boolean enabled = cd.isMergeable();
 
     if (submitProblems != null) {
       return new UiAction.Description()
@@ -476,14 +463,6 @@ public class Submit
     return submitter;
   }
 
-  private List<ChangeData> getChangesByTopic(String topic) {
-    try {
-      return queryProvider.get().byTopicOpen(topic);
-    } catch (StorageException e) {
-      throw new StorageRuntimeException(e);
-    }
-  }
-
   public static class CurrentRevision implements RestModifyView<ChangeResource, SubmitInput> {
     private final Submit submit;
     private final ChangeJson.Factory json;