tree df454d7590d1dac6576194feb1ed8dec63299a3d
parent 32f3694c705a1937deaaab51a9e9c44c3fdc0a1b
author Edwin Kempin <ekempin@google.com> 1626789573 +0200
committer Edwin Kempin <ekempin@google.com> 1626861682 +0200

ApprovalContext: Drop checkState that verified a wrong assumption

This code made the wrong assumption that a consecutive patch set always
has a patch set number that is by 1 greater than the number of the
previous patch set.

For old changes it can be that there are gaps in the patch set numbers:

1. Due to non-atomic patch set creations with ReviewDb:
   If a failure happened after creating the patch set ref and before
   the patch set was created in the database, any new attempt to create
   the patch was using the next number that was free according to the
   existing patch set refs.

2. Due to deleted draft patch sets.

If there was a gap in the patch set numbers and approvals were supposed
to be copied over this gap, loading the change failed with an ISE [1].

A similar exception occurred when such a change was found as part of a
change query [2].

To fix this drop the checkState that makes the wrong assumption.

[1]
com.google.gerrit.exceptions.StorageException: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
  at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:452)
  at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:302)
  at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:298)
  at com.google.gerrit.server.restapi.change.GetChange.apply(GetChange.java:95)
  at com.google.gerrit.server.restapi.change.GetDetail.apply(GetDetail.java:65)
  at com.google.gerrit.server.restapi.change.GetDetail.apply(GetDetail.java:29)
  at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestReadViewWithRetry$6(RestApiServlet.java:867)
  at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
  at com.github.rholder.retry.Retryer.call(Retryer.java:160)
  at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:561)
  at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:504)
  at com.google.gerrit.server.update.RetryableAction.call(RetryableAction.java:172)
  at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:958)
  at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestReadViewWithRetry(RestApiServlet.java:862)
  at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:530)
  at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
  ...

[2]
ChangeJson.toChangeInfos:516 Omitting corrupt change 8267 from results
com.google.gerrit.exceptions.StorageException: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
  at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:452)
  at com.google.gerrit.server.change.ChangeJson.toChangeInfos(ChangeJson.java:510)
  at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:319)
  at com.google.gerrit.server.restapi.change.QueryChanges.query(QueryChanges.java:190)
  at com.google.gerrit.server.restapi.change.QueryChanges.apply(QueryChanges.java:149)
  at com.google.gerrit.server.restapi.change.QueryChanges.apply(QueryChanges.java:48)
  at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestReadViewWithRetry$6(RestApiServlet.java:867)
  at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
  at com.github.rholder.retry.Retryer.call(Retryer.java:160)
  at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:561)
  at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:504)
  at com.google.gerrit.server.update.RetryableAction.call(RetryableAction.java:172)
  at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:958)
  at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestReadViewWithRetry(RestApiServlet.java:862)
  at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:530)
  at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
  ...
Caused by: com.google.common.util.concurrent.UncheckedExecutionException: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
  at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2052)
  at com.google.common.cache.LocalCache.get(LocalCache.java:3972)
  at com.google.common.cache.LocalCache$LocalManualCache.get(LocalCache.java:4869)
  at com.google.devtools.gerritcodereview.common.cache.BigtableBackedCache.get(BigtableBackedCache.java:91)
  at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache.get(SiteCacheFactory.java:374)
  at com.google.gerrit.server.approval.ApprovalCacheImpl.get(ApprovalCacheImpl.java:74)
  at com.google.gerrit.server.approval.ApprovalsUtil.byPatchSet(ApprovalsUtil.java:348)
  at com.google.gerrit.server.git.MergeUtil.safeGetApprovals(MergeUtil.java:653)
  at com.google.gerrit.server.git.MergeUtil.createDetailedCommitMessage(MergeUtil.java:558)
  at com.google.gerrit.server.git.MergeUtil.createCommitMessageOnSubmit(MergeUtil.java:640)
  at com.google.gerrit.server.change.RevisionJson.toRevisionInfo(RevisionJson.java:317)
  at com.google.gerrit.server.change.RevisionJson.getRevisions(RevisionJson.java:238)
  at com.google.gerrit.server.change.ChangeJson.toChangeInfoImpl(ChangeJson.java:723)
  at com.google.gerrit.server.change.ChangeJson.toChangeInfo(ChangeJson.java:571)
  at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:444)
  ... 220 more
Caused by: java.lang.IllegalStateException: approvals can only be copied to the next consecutive patch set. got: 8267,1, 8267,3
  at com.google.common.base.Preconditions.checkState(Preconditions.java:832)
  at com.google.gerrit.server.query.approval.ApprovalContext.create(ApprovalContext.java:47)
  at com.google.gerrit.server.approval.ApprovalInference.canCopyBasedOnCopyCondition(ApprovalInference.java:319)
  at com.google.gerrit.server.approval.ApprovalInference.getForPatchSetWithoutNormalization(ApprovalInference.java:424)
  at com.google.gerrit.server.approval.ApprovalInference.forPatchSet(ApprovalInference.java:116)
  at com.google.gerrit.server.approval.ApprovalCacheImpl$Loader.load(ApprovalCacheImpl.java:105)
  at com.google.gerrit.server.approval.ApprovalCacheImpl$Loader.load(ApprovalCacheImpl.java:88)
  at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache.lambda$get$0(SiteCacheFactory.java:372)
  at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache$RecordingCallable.lambda$new$0(SiteCacheFactory.java:571)
  at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache$BulkRecordingCallable.call(SiteCacheFactory.java:609)
  at com.google.devtools.gerritcodereview.server.cache.SiteCacheFactory$SiteCache$RecordingCallable.call(SiteCacheFactory.java:577)
  at com.google.devtools.gerritcodereview.common.cache.BigtableBackedCache$Loader.call(BigtableBackedCache.java:216)
  at com.google.common.cache.LocalCache$LocalManualCache$1.load(LocalCache.java:4874)
  at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3539)
  at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2279)
  at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2156)
  at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2046)
  ... 234 more

Issue: Google b/194181275
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I995e213054a252f46c68c8fe53d0a85691f06adb