opendev/gerrit
Code Issues Proposed changes

Submit: Differentiate between conflicts and server errors

Browse Source 
If a change cannot be submitted we currently always throw an
IntegrationException that is mapped to ResouceConflictException, which
the calling user sees as '409 Conflict'.

IntegrationException is thrown in 2 cases:
1. the change cannot be submitted due to conflicts
2. the change cannot be submitted due to an error in Gerrit

For 2. we should not return '409 Conflict' but rather '500 Internal
Server Error'. Returning '409 Conflict' for server errors is bad because
our metric and alerting system is not seeing them as errors and we do
leak internal messages to the calling user.

To fix this we introduce an IntegrateConflictException as subclass of
ResourceConflictException that is thrown if there is a conflict on merge
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 quite a bit.

We do have some places now that catch StorageException and then throw a
new StorageException. I left this in place because I think the message
of the newly thrown StorageException is of value.

SubmoduleException is still mapped to '409 Conflict', but for this
exception we have the same issue. This will be addressed in a follow-up
change.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ifdc9d122b54f190ac37a0c3e8e10f9a6b0ec4139
This commit is contained in:
Edwin Kempin
2020-03-06 14:45:35 +01:00
parent fe22b7dfae
commit 9417d69c13
22 changed files with 116 additions and 154 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/server/query/change/ConflictsPredicate.java
View File

@@ -35,7 +35,6 @@ import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments;
import com.google.gerrit.server.submit.IntegrationException;
import com.google.gerrit.server.submit.SubmitDryRun;
import java.io.IOException;
import java.util.ArrayList;
@@ -163,7 +162,7 @@ public class ConflictsPredicate {
args.conflictsCache.put(conflictsKey, conflicts);
return conflicts;
}
} catch (IntegrationException | NoSuchProjectException | StorageException | IOException e) {
} catch (NoSuchProjectException | StorageException | IOException e) {
ObjectId finalOther = other;
warnWithOccasionalStackTrace(
e,
@@ -181,8 +180,7 @@ public class ConflictsPredicate {
return 5;
}
private Set<RevCommit> getAlreadyAccepted(Repository repo, RevWalk rw)
throws IntegrationException {
private Set<RevCommit> getAlreadyAccepted(Repository repo, RevWalk rw) {
try {
Set<RevCommit> accepted = new HashSet<>();
SubmitDryRun.addCommits(changeDataCache.getAlreadyAccepted(repo), rw, accepted);
@@ -192,7 +190,7 @@ public class ConflictsPredicate {
}
return accepted;
} catch (StorageException | IOException e) {
throw new IntegrationException("Failed to determine already accepted commits.", e);
throw new StorageException("Failed to determine already accepted commits.", e);
}
}
}