Avoid work in Optional.orElse() calls

Any code inside the orElse() will always be called, even if the result
isn't used because the optional is present. This is wasteful at a
minimum and can be actively harmful (as seen in [1]) because of side
effects or performance impacts. Fix that by replacing all orElse() calls
that create new instances or do non-constant work with either a call to
orElseGet() or an if statement.

It's possible this will result in performance improvements, but I didn't
attempt to measure any, so I'm not including a release note.

It would be nice if there were an ErrorProne checker for this, but one
doesn't exist yet.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/417915/comment/0ea287cd_bfecb7f2/

Change-Id: Icab4115998fb4f3787ca74c0890949cb3ce5c6a6
Release-Notes: skip

14 files changed, 24 insertions, 22 deletions

diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index a9ad3f237b..3bbc74ee13 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -332,7 +332,7 @@ public class GerritServer implements AutoCloseable {
     String configuredIndexBackend = cfg.getString("index", null, "type");
     if (configuredIndexBackend == null) {
       // Propagate index type to pgms that run off of the gerrit.config file on local disk.
-      IndexType indexType = IndexType.fromEnvironment().orElse(new IndexType("fake"));
+      IndexType indexType = IndexType.fromEnvironment().orElseGet(() -> new IndexType("fake"));
       gerritConfig.setString("index", null, "type", indexType.isLucene() ? "lucene" : "fake");
     }
     gerritConfig.save();
@@ -509,7 +509,7 @@ public class GerritServer implements AutoCloseable {
     IndexType indexType =
         (configuredIndexBackend != null)
             ? new IndexType(configuredIndexBackend)
-            : IndexType.fromEnvironment().orElse(new IndexType("fake"));
+            : IndexType.fromEnvironment().orElseGet(() -> new IndexType("fake"));
     daemon.setIndexModule(createIndexModule(indexType, baseConfig, testIndexModule));
 
     daemon.setEnableHttpd(desc.httpd());
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java
index dcf1158e14..a37c2babbe 100644
--- a/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java
@@ -78,7 +78,7 @@ public class GroupOperationsImpl implements GroupOperations {
 
   private InternalGroupCreation toInternalGroupCreation(TestGroupCreation groupCreation) {
     AccountGroup.Id groupId = AccountGroup.id(seq.nextGroupId());
-    String groupName = groupCreation.name().orElse("group-with-id-" + groupId.get());
+    String groupName = groupCreation.name().orElseGet(() -> "group-with-id-" + groupId.get());
     AccountGroup.UUID groupUuid = GroupUuid.make(groupName, serverIdent);
     AccountGroup.NameKey nameKey = AccountGroup.nameKey(groupName);
     return InternalGroupCreation.builder()
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
index deeb84309f..1014a0f7ad 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -85,7 +85,7 @@ public class ProjectOperationsImpl implements ProjectOperations {
   }
 
   private Project.NameKey createNewProject(TestProjectCreation projectCreation) throws Exception {
-    String name = projectCreation.name().orElse(RandomStringUtils.randomAlphabetic(8));
+    String name = projectCreation.name().orElseGet(() -> RandomStringUtils.randomAlphabetic(8));
 
     CreateProjectArgs args = new CreateProjectArgs();
     args.setProjectName(name);
diff --git a/java/com/google/gerrit/httpd/raw/StaticModule.java b/java/com/google/gerrit/httpd/raw/StaticModule.java
index 15dcf42e0e..1e76f215e9 100644
--- a/java/com/google/gerrit/httpd/raw/StaticModule.java
+++ b/java/com/google/gerrit/httpd/raw/StaticModule.java
@@ -242,7 +242,7 @@ public class StaticModule extends ServletModule {
         @GerritServerConfig Config cfg,
         GerritApi gerritApi,
         ExperimentFeatures experimentFeatures) {
-      String cdnPath = options.devCdn().orElse(cfg.getString("gerrit", null, "cdnPath"));
+      String cdnPath = options.devCdn().orElseGet(() -> cfg.getString("gerrit", null, "cdnPath"));
       String faviconPath = cfg.getString("gerrit", null, "faviconPath");
       return new IndexServlet(canonicalUrl, cdnPath, faviconPath, gerritApi, experimentFeatures);
     }
diff --git a/java/com/google/gerrit/server/DeadlineChecker.java b/java/com/google/gerrit/server/DeadlineChecker.java
index f41b1e3c3d..9b7ffe6c3f 100644
--- a/java/com/google/gerrit/server/DeadlineChecker.java
+++ b/java/com/google/gerrit/server/DeadlineChecker.java
@@ -180,12 +180,14 @@ public class DeadlineChecker implements RequestStateProvider {
     this.timeoutName =
         clientedProvidedTimeout
             .map(clientTimeout -> "client.timeout")
-            .orElse(
-                serverSideDeadline
-                    .map(serverDeadline -> serverDeadline.id() + ".timeout")
-                    .orElse("timeout"));
+            .orElseGet(
+                () ->
+                    serverSideDeadline
+                        .map(serverDeadline -> serverDeadline.id() + ".timeout")
+                        .orElse("timeout"));
     this.timeout =
-        clientedProvidedTimeout.orElse(serverSideDeadline.map(ServerDeadline::timeout).orElse(0L));
+        clientedProvidedTimeout.orElseGet(
+            () -> serverSideDeadline.map(ServerDeadline::timeout).orElse(0L));
     this.deadline = timeout > 0 ? Optional.of(start + timeout) : Optional.empty();
   }
 
diff --git a/java/com/google/gerrit/server/account/AccountCacheImpl.java b/java/com/google/gerrit/server/account/AccountCacheImpl.java
index 66a36f6595..d306ad0e92 100644
--- a/java/com/google/gerrit/server/account/AccountCacheImpl.java
+++ b/java/com/google/gerrit/server/account/AccountCacheImpl.java
@@ -98,7 +98,7 @@ public class AccountCacheImpl implements AccountCache {
 
   @Override
   public AccountState getEvenIfMissing(Account.Id accountId) {
-    return get(accountId).orElse(missing(accountId));
+    return get(accountId).orElseGet(() -> missing(accountId));
   }
 
   @Override
diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java
index 891a467577..5edba08eaa 100644
--- a/java/com/google/gerrit/server/account/AccountManager.java
+++ b/java/com/google/gerrit/server/account/AccountManager.java
@@ -195,7 +195,7 @@ public class AccountManager {
           "Unable to deactivate account %s",
           authRequest
               .getUserName()
-              .orElse(" for external ID key " + authRequest.getExternalIdKey().get()));
+              .orElseGet(() -> " for external ID key " + authRequest.getExternalIdKey().get()));
     }
   }
 
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index 1199be5657..84ea730c57 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -214,7 +214,7 @@ public class DeleteReviewerOp extends ReviewerOp {
             reviewerDeleted.fire(
                 ctx.getChangeData(currChange),
                 patchSet,
-                accountCache.get(reviewer.id()).orElse(AccountState.forAccount(reviewer)),
+                accountCache.get(reviewer.id()).orElseGet(() -> AccountState.forAccount(reviewer)),
                 ctx.getAccount(),
                 mailMessage,
                 newApprovals,
diff --git a/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java b/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
index 4f6094e685..58c3eb10b9 100644
--- a/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
+++ b/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
@@ -54,7 +54,7 @@ public class DefaultChangeReportFormatter implements ChangeReportFormatter {
         urlFormatter
             .get()
             .getChangeViewUrl(c.getProject(), c.getId())
-            .orElse(c.getId().toString()));
+            .orElseGet(() -> c.getId().toString()));
   }
 
   protected String cropSubject(String subject) {
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 0f5e3bc1ba..dbe0fa8857 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -2019,10 +2019,10 @@ class ReceiveCommits {
       magicBranch.dest = BranchNameKey.create(project.getNameKey(), ref);
       magicBranch.perm = permissions.ref(ref);
 
-      Optional<AuthException> err =
-          checkRefPermission(magicBranch.perm, RefPermission.READ)
-              .map(Optional::of)
-              .orElse(checkRefPermission(magicBranch.perm, RefPermission.CREATE_CHANGE));
+      Optional<AuthException> err = checkRefPermission(magicBranch.perm, RefPermission.READ);
+      if (err.isEmpty()) {
+        err = checkRefPermission(magicBranch.perm, RefPermission.CREATE_CHANGE);
+      }
       if (err.isPresent()) {
         rejectProhibited(cmd, err.get());
         return;
diff --git a/java/com/google/gerrit/server/group/db/AuditLogFormatter.java b/java/com/google/gerrit/server/group/db/AuditLogFormatter.java
index 235ca4f613..3ba087e315 100644
--- a/java/com/google/gerrit/server/group/db/AuditLogFormatter.java
+++ b/java/com/google/gerrit/server/group/db/AuditLogFormatter.java
@@ -167,7 +167,7 @@ public class AuditLogFormatter {
         .map(Account::getName)
         // Historically, the database did not enforce relational integrity, so it is
         // possible for groups to have non-existing members.
-        .orElse("No Account for Id #" + accountId);
+        .orElseGet(() -> "No Account for Id #" + accountId);
   }
 
   private PersonIdent getParsableAuthorIdent(
diff --git a/java/com/google/gerrit/server/patch/PatchFile.java b/java/com/google/gerrit/server/patch/PatchFile.java
index 7a8180bd13..c3a6807fac 100644
--- a/java/com/google/gerrit/server/patch/PatchFile.java
+++ b/java/com/google/gerrit/server/patch/PatchFile.java
@@ -61,7 +61,7 @@ public class PatchFile {
             .filter(f -> f.getKey().equals(fileName))
             .map(Map.Entry::getValue)
             .findFirst()
-            .orElse(FileDiffOutput.empty(fileName, ObjectId.zeroId(), ObjectId.zeroId()));
+            .orElseGet(() -> FileDiffOutput.empty(fileName, ObjectId.zeroId(), ObjectId.zeroId()));
 
     if (Patch.PATCHSET_LEVEL.equals(fileName)) {
       aTree = null;
diff --git a/java/com/google/gerrit/server/project/Reachable.java b/java/com/google/gerrit/server/project/Reachable.java
index 342c2bcc06..c935adf079 100644
--- a/java/com/google/gerrit/server/project/Reachable.java
+++ b/java/com/google/gerrit/server/project/Reachable.java
@@ -74,7 +74,7 @@ public class Reachable {
       Collection<Ref> filtered =
           optionalUserProvider
               .map(permissionBackend::user)
-              .orElse(permissionBackend.currentUser())
+              .orElseGet(() -> permissionBackend.currentUser())
               .project(project)
               .filter(refs, repo, RefFilterOptions.defaults());
       Collection<RevCommit> visible = new ArrayList<>();
diff --git a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
index bea5eaad50..a9ceddb076 100644
--- a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
+++ b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
@@ -141,7 +141,7 @@ public class AbstractGroupTest {
               return GroupConfig.loadForGroup(allUsersName, allUsersRepo, uuid)
                   .getLoadedGroup()
                   .map(InternalGroup::getName)
-                  .orElse("Group " + uuid);
+                  .orElseGet(() -> "Group " + uuid);
             } catch (IOException | ConfigInvalidException e) {
               return "Group " + uuid;
             }