opendev/gerrit
Code Issues Proposed changes

Throw a StorageException in ProjectCache#get instead of covering up

Browse Source 
ProjectCache has a dangerous behavior in it's #get method: It covers up
exceptions by returning NULL - the same value it returns when the
project was not found. This makes it impossible for callers to
distinguish between "project not found" and "exception".

That is dangerous because ProjectState contains permissions and
pretenting a project isn't there - for example when traversing hierarchy
can lead to wrong decisions.

This is the first change of a series of changes. Here, we are marking
all interfaces to get a ProjectState besides #get @Deprecated because
they will be removed in follow-up commits. In addition, we are throwing
a storage exception in case of a failure. This is a RuntimeException
that will make callers come to a grinding halt - which is what we want.

In follow-up changes, we'll move the #get interface to return an
Optional<ProjectState> and force callers to make an explicit decision
about "project not found". In some cases (in API handlers) the correct
handling might be throwing a RestAPIException, in others (down in
storage code) an IllegalStateException the best reaction.

Change-Id: Id5ef10498ac956103aabfc7d59585c6b32b1b801
This commit is contained in:
Patrick Hiesel
2020-03-04 11:10:45 +01:00
parent b7fade824e
commit 8d6c9ade61
2 changed files with 10 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
java/com/google/gerrit/server/project/ProjectCache.java
View File

@@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableSortedSet;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import java.io.IOException;
import java.util.Set;
@@ -33,11 +34,11 @@ public interface ProjectCache {
* Get the cached data for a project by its unique name.
*
* @param projectName name of the project.
* @return the cached data; null if no such project exists, projectName is null or an error
* occurred.
* @return the cached data; null if no such project exists or the projectName is null
* @throws StorageException when there was an error.
* @see #checkedGet(com.google.gerrit.entities.Project.NameKey)
*/
ProjectState get(@Nullable Project.NameKey projectName);
ProjectState get(@Nullable Project.NameKey projectName) throws StorageException;
/**
* Get the cached data for a project by its unique name.
@@ -45,7 +46,9 @@ public interface ProjectCache {
* @param projectName name of the project.
* @throws IOException when there was an error.
* @return the cached data; null if no such project exists or projectName is null.
* @deprecated use {@link #get(Project.NameKey)} instead.
*/
@Deprecated
ProjectState checkedGet(@Nullable Project.NameKey projectName) throws IOException;
/**
@@ -56,7 +59,9 @@ public interface ProjectCache {
* @throws Exception in case of any error (strict = true) or only for I/O or other internal
* errors.
* @return the cached data or null when strict = false
* @deprecated use {@link #get(Project.NameKey)} instead.
*/
@Deprecated
ProjectState checkedGet(Project.NameKey projectName, boolean strict) throws Exception;
/**

4
java/com/google/gerrit/server/project/ProjectCacheImpl.java
View File

@@ -26,6 +26,7 @@ import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.project.ProjectIndexer;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.metrics.Description;
@@ -148,8 +149,7 @@ public class ProjectCacheImpl implements ProjectCache {
try {
return checkedGet(projectName);
} catch (IOException e) {
logger.atWarning().withCause(e).log("Cannot read project %s", projectName);
return null;
throw new StorageException("project state not available", e);
}
}