opendev/gerrit
Code Issues Proposed changes

Exit early from Submit.getDescription for non-current resources

Browse Source 
If a change is closed or the current patch set isn't eligible for
submission exit the method early, without making a test call to the
PermissionBackend.  It's acceptable to return null from the method
instead of building a description with setVisible(false). Callers
building the action map always skip null results.

Use testOrFalse to simplify the error handling when we do check the
PermissionBackend.  This is still performed immediately to
short-circuit away from much more expensive checks related to topics.

Change-Id: I7d4a0daa03a4d50f4bd5e97965d3d69a86d2ba0b
This commit is contained in:
Shawn Pearce
2017-08-28 16:52:14 -07:00
parent 3bb66d66b0
commit 9b8bb464b1
1 changed files with 9 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@@ -313,31 +313,24 @@ public class Submit
@Override
public UiAction.Description getDescription(RevisionResource resource) {
Change change = resource.getChange();
String topic = change.getTopic();
if (!change.getStatus().isOpen()
|| !resource.isCurrent()
|| resource.getPatchSet().isDraft()
|| !resource.permissions().testOrFalse(ChangePermission.SUBMIT)) {
return null; // submit not visible
}
ReviewDb db = dbProvider.get();
ChangeData cd = changeDataFactory.create(db, resource.getControl());
boolean visible;
try {
visible =
change.getStatus().isOpen()
&& resource.isCurrent()
&& !resource.getPatchSet().isDraft()
&& resource.permissions().test(ChangePermission.SUBMIT);
MergeOp.checkSubmitRule(cd, false);
} catch (ResourceConflictException e) {
visible = false;
} catch (PermissionBackendException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException("Could not check submit permission", e);
return null; // submit not visible
} catch (OrmException e) {
log.error("Error checking if change is submittable", e);
throw new OrmRuntimeException("Could not determine problems for the change", e);
}
if (!visible) {
return new UiAction.Description().setLabel("").setTitle("").setVisible(false);
}
ChangeSet cs;
try {
cs = mergeSuperSet.get().completeChangeSet(db, cd.change(), resource.getControl().getUser());
@@ -346,6 +339,7 @@ public class Submit
"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();