Files
gerrit/java/com/google/gerrit/server/restapi/change/PutDraftComment.java
Edwin Kempin cec334a06f Retry requests in RestApiServlet, no more retrying for Java API
Instead of implementing retrying of REST requests in base classes of
REST endpoints, implement retrying of requests once in RestApiServlet.

This has the following advantages:
- We now automatically retry all modifying REST requests (not only
  change related modifying REST requests)
- The implementations of the REST endpoints get easier (no longer need
  to extend a Retrying* base class, RetryHelper no longer needs to be
  injected).
- It's no longer possible to forget to enable retrying for a REST
  endpoint (e.g. by just implementing the interface without extending a
  Retrying* base class)
- Follow-up changes can easily add retrying for other types of requests
  (e.g. read requests or create/delete on collection). Before this
  change we would have needed to add another Retrying* base class and
  adapt all relevant REST endpoints to extend that base class.
- In case of auto-retry-with-trace we do not need a complicated way to
  return the trace ID from the REST endpoint implementation back to
  RestApiServlet (means we can simplify the existing code in a
  follow-up change).
- In RestApiServlet we have more information about the REST endpoint
  available and may include this information into traces on auto-retry
  (e.g. URL of the invoked REST endpoint).

One important change in behavior with this change is that we are no
longer retrying calls that are made through the Java extension API. This
is actually good, because it prevents amplifying of retries when this
API is used by plugins. Examples:

- A plugin implements a new REST endpoint and uses the Java extension
  API to implement it. With the old behavior we would have retrying on
  the REST endpoint and retrying on each call to the Java extension API.
  However we would prefer to have retrying only on one level.

- A plugin implements an extension point that is invoked from a REST
  endpoint and uses the Java extension API to implement it. With the old
  behavior we would have retrying on the REST endpoint and retrying on
  each call to the Java extension API. However we would prefer to have
  retrying only on one level.

In addition it's also better for our tests (that mostly use the Java
extension API) that retrying is not done, since the retry logic may
obfuscate flaky behavior.

RetryingRestModifyView and RetryingRestCollectionModifyView are now no
longer needed and are removed. There was one small complication with
removing RetryingRestModifyView. NoteDbUtil.guessRestApiHandler() was
using this class as a marker to find the REST endpoint class from a
stacktrace if the call was done through the Java API and not via REST.
Instead of looking for RetryingRestModifyView in the stacktrace, we now
watch out for a "com.google.gerrit.server.api.*ApiImpl" class. This may
look a little fragile, but API classes are not supposed to change and if
they do and this breaks we have a test that catches this
(ReflogIT#guessRestApiInReflog). For reference here are sample
stacktraces from before [1] and after this change [2].

[1]
com.google.gerrit.server.notedb.NoteDbUtil.guessRestApiHandler(NoteDbUtil.java:74)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:340)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:305)
  at com.google.gerrit.server.update.BatchUpdate$ChangesHandle.execute(BatchUpdate.java:508)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:141)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:357)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:361)
  at com.google.gerrit.server.restapi.change.PutTopic.applyImpl(PutTopic.java:78)
  at com.google.gerrit.server.restapi.change.PutTopic.applyImpl(PutTopic.java:1)
  at com.google.gerrit.server.update.RetryingRestModifyView.lambda$1(RetryingRestModifyView.java:43)
  at com.google.gerrit.server.update.RetryHelper.lambda$1(RetryHelper.java:274)
  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:375)
  at com.google.gerrit.server.update.RetryHelper.executeWithAttemptAndTimeoutCount(RetryHelper.java:353)
  at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:257)
  at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:272)
  at com.google.gerrit.server.update.RetryingRestModifyView.apply(RetryingRestModifyView.java:43)
  at com.google.gerrit.server.api.changes.ChangeApiImpl.topic(ChangeApiImpl.java:430)
  at com.google.gerrit.acceptance.server.project.ReflogIT.guessRestApiInReflog(ReflogIT.java:60)
  ...

[2]
com.google.gerrit.server.notedb.NoteDbUtil.guessRestApiHandler(NoteDbUtil.java:72)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:340)
  at com.google.gerrit.server.notedb.NoteDbUpdateManager.execute(NoteDbUpdateManager.java:305)
  at com.google.gerrit.server.update.BatchUpdate$ChangesHandle.execute(BatchUpdate.java:508)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:141)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:357)
  at com.google.gerrit.server.update.BatchUpdate.execute(BatchUpdate.java:361)
  at com.google.gerrit.server.restapi.change.PutTopic.apply(PutTopic.java:77)
  at com.google.gerrit.server.api.changes.ChangeApiImpl.topic(ChangeApiImpl.java:430)
  at com.google.gerrit.acceptance.server.project.ReflogIT.guessRestApiInReflog(ReflogIT.java:60)
  ...

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ief5ee42aea9aad575e6927ee54e0b770fd14dc88
2019-11-11 16:20:15 -08:00

164 lines
6.3 KiB
Java

// Copyright (C) 2012 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.DraftCommentResource;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.sql.Timestamp;
import java.util.Collections;
import java.util.Optional;
@Singleton
public class PutDraftComment implements RestModifyView<DraftCommentResource, DraftInput> {
private final BatchUpdate.Factory updateFactory;
private final DeleteDraftComment delete;
private final CommentsUtil commentsUtil;
private final PatchSetUtil psUtil;
private final Provider<CommentJson> commentJson;
private final PatchListCache patchListCache;
@Inject
PutDraftComment(
BatchUpdate.Factory updateFactory,
DeleteDraftComment delete,
CommentsUtil commentsUtil,
PatchSetUtil psUtil,
Provider<CommentJson> commentJson,
PatchListCache patchListCache) {
this.updateFactory = updateFactory;
this.delete = delete;
this.commentsUtil = commentsUtil;
this.psUtil = psUtil;
this.commentJson = commentJson;
this.patchListCache = patchListCache;
}
@Override
public Response<CommentInfo> apply(DraftCommentResource rsrc, DraftInput in)
throws RestApiException, UpdateException, PermissionBackendException {
if (in == null || in.message == null || in.message.trim().isEmpty()) {
return delete.apply(rsrc, null);
} else if (in.id != null && !rsrc.getId().equals(in.id)) {
throw new BadRequestException("id must match URL");
} else if (in.line != null && in.line < 0) {
throw new BadRequestException("line must be >= 0");
} else if (in.line != null && in.range != null && in.line != in.range.endLine) {
throw new BadRequestException("range endLine must be on the same line as the comment");
}
try (BatchUpdate bu =
updateFactory.create(rsrc.getChange().getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
Op op = new Op(rsrc.getComment().key, in);
bu.addOp(rsrc.getChange().getId(), op);
bu.execute();
return Response.ok(
commentJson.get().setFillAccounts(false).newCommentFormatter().format(op.comment));
}
}
private class Op implements BatchUpdateOp {
private final Comment.Key key;
private final DraftInput in;
private Comment comment;
private Op(Comment.Key key, DraftInput in) {
this.key = key;
this.in = in;
}
@Override
public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, PatchListNotAvailableException {
Optional<Comment> maybeComment =
commentsUtil.getDraft(ctx.getNotes(), ctx.getIdentifiedUser(), key);
if (!maybeComment.isPresent()) {
// Disappeared out from under us. Can't easily fall back to insert,
// because the input might be missing required fields. Just give up.
throw new ResourceNotFoundException("comment not found: " + key);
}
Comment origComment = maybeComment.get();
comment = new Comment(origComment);
// Copy constructor preserved old real author; replace with current real
// user.
ctx.getUser().updateRealAccountId(comment::setRealAuthor);
PatchSet.Id psId = PatchSet.id(ctx.getChange().getId(), origComment.key.patchSetId);
ChangeUpdate update = ctx.getUpdate(psId);
PatchSet ps = psUtil.get(ctx.getNotes(), psId);
if (ps == null) {
throw new ResourceNotFoundException("patch set not found: " + psId);
}
if (in.path != null && !in.path.equals(origComment.key.filename)) {
// Updating the path alters the primary key, which isn't possible.
// Delete then recreate the comment instead of an update.
commentsUtil.deleteComments(update, Collections.singleton(origComment));
comment.key.filename = in.path;
}
setCommentCommitId(comment, patchListCache, ctx.getChange(), ps);
commentsUtil.putComments(
update, Comment.Status.DRAFT, Collections.singleton(update(comment, in, ctx.getWhen())));
return true;
}
}
private static Comment update(Comment e, DraftInput in, Timestamp when) {
if (in.side != null) {
e.side = in.side();
}
if (in.inReplyTo != null) {
e.parentUuid = Url.decode(in.inReplyTo);
}
e.setLineNbrAndRange(in.line, in.range);
e.message = in.message.trim();
e.writtenOn = when;
if (in.tag != null) {
// TODO(dborowitz): Can we support changing tags via PUT?
e.tag = in.tag;
}
if (in.unresolved != null) {
e.unresolved = in.unresolved;
}
return e;
}
}