diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index db358f3bbf..bd94789e2c 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -300,6 +300,11 @@ default. Optional fields are: * `WEB_LINKS`: include the `web_links` field. -- +[[check]] +-- +* `CHECK`: include potential problems with the change. +-- + .Request ---- GET /changes/?q=97&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=CURRENT_FILES&o=DOWNLOAD_COMMANDS HTTP/1.0 @@ -1143,7 +1148,12 @@ Adds or updates the change in the secondary index. -- Performs consistency checks on the change, and returns a -link:#check-result[CheckResult] entity. +link:#change-info[ChangeInfo] entity with the `problems` field set to a +list of link:#problem-info[ProblemInfo] entities. + +Depending on the type of problem, some fields not marked optional may be +missing from the result. At least `id`, `project`, `branch`, and +`_number` will be present. .Request ---- @@ -1158,26 +1168,80 @@ link:#check-result[CheckResult] entity. )]}' { - "change": { - "id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940", - "project": "myProject", - "branch": "master", - "change_id": "I8473b95934b5732ac55d26311a706c9c2bde9940", - "subject": "Implementing Feature X", - "status": "NEW", - "created": "2013-02-01 09:59:32.126000000", - "updated": "2013-02-21 11:16:36.775000000", - "mergeable": true, - "insertions": 34, - "deletions": 101, - "_sortkey": "0023412400000f7d", - "_number": 3965, - "owner": { - "name": "John Doe" - } + "id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940", + "project": "myProject", + "branch": "master", + "change_id": "I8473b95934b5732ac55d26311a706c9c2bde9940", + "subject": "Implementing Feature X", + "status": "NEW", + "created": "2013-02-01 09:59:32.126000000", + "updated": "2013-02-21 11:16:36.775000000", + "mergeable": true, + "insertions": 34, + "deletions": 101, + "_sortkey": "0023412400000f7d", + "_number": 3965, + "owner": { + "name": "John Doe" }, - "messages": [ - "Current patch set 1 not found" + "problems": [ + { + "message": "Current patch set 1 not found" + } + ] + } +---- + +[[fix-change]] +=== Fix change +-- +'POST /changes/link:#change-id[\{change-id\}]/check' +-- + +Performs consistency checks on the change as with link:#check-change[GET +/check], and additionally fixes any problems that can be fixed +automatically. The returned field values reflect any fixes. + +Only the change owner, a project owner, or an administrator may fix changes. + +.Request +---- + POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/check HTTP/1.0 +---- + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json;charset=UTF-8 + + )]}' + { + "id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940", + "project": "myProject", + "branch": "master", + "change_id": "I8473b95934b5732ac55d26311a706c9c2bde9940", + "subject": "Implementing Feature X", + "status": "MERGED", + "created": "2013-02-01 09:59:32.126000000", + "updated": "2013-02-21 11:16:36.775000000", + "mergeable": true, + "insertions": 34, + "deletions": 101, + "_sortkey": "0023412400000f7d", + "_number": 3965, + "owner": { + "name": "John Doe" + }, + "problems": [ + { + "message": "Current patch set 2 not found" + }, + { + "message": "Patch set 1 (1eee2c9d8f352483781e772f35dc586a69ff5646) is merged into destination ref master (1eee2c9d8f352483781e772f35dc586a69ff5646), but change status is NEW", + "status": FIXED, + "outcome": "Marked change as merged" + } ] } ---- @@ -3259,6 +3323,9 @@ if link:#all-revisions[all revisions] are requested. |`_more_changes` |optional, not set if `false`| Whether the query would deliver more results if not limited. + Only set on either the last or the first change that is returned. +|`problems` |optional| +A list of link:#problem-info[ProblemInfo] entities describing potential +problems with this change. Only set if link:#check[CHECK] is set. |================================== [[related-changes-info]] @@ -3978,22 +4045,23 @@ the commit message within a change edit. |`message` ||New commit message. |=========================== -[[check-result]] -=== CheckResult -The `CheckResult` entity contains the results of a consistency check on -a change. +[[problem-info]] +=== ProblemInfo +The `ProblemInfo` entity contains a description of a potential consistency problem +with a change. These are not related to the code review process, but rather +indicate some inconsistency in Gerrit's database or repository metadata related +to the enclosing change. -[options="header",cols="1,6"] +[options="header",cols="1,^1,5"] |=========================== -|Field Name|Description -|`change`| -link:#change-info[ChangeInfo] entity containing information about the change, -as in link:#get-change[Get Change] with no options. Some fields not marked -optional may be missing if a consistency check failed, but at least -`id`, `project`, `branch`, and `_number` will be present. -|`messages`| -List of messages describing potential problems with the change. May be -empty if no problems were found. +|Field Name||Description +|`message` ||Plaintext message describing the problem with the change. +|`status` |optional| +The status of fixing the problem (`FIXED`, `FIX_FAILED`). Only set if a +fix was attempted. +|`outcome` |optional| +If `status` is set, an additional plaintext message describing the +outcome of the fix. |=========================== GERRIT diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index ae7e94be6c..ec213289a2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -301,4 +301,17 @@ public class ChangeIT extends AbstractDaemonTest { .id(r.getChangeId()) .topic()).isEqualTo(""); } + + @Test + public void check() throws Exception { + PushOneCommit.Result r = createChange(); + assertThat(gApi.changes() + .id(r.getChangeId()) + .get() + .problems).isNull(); + assertThat(gApi.changes() + .id(r.getChangeId()) + .get(EnumSet.of(ListChangesOption.CHECK)) + .problems).isEmpty(); + } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java new file mode 100644 index 0000000000..db45e4c8b0 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java @@ -0,0 +1,88 @@ +// Copyright (C) 2014 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.acceptance.api.change; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.extensions.api.changes.FixInput; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ChangeStatus; +import com.google.gerrit.extensions.common.ProblemInfo; +import com.google.gerrit.reviewdb.client.Change; + +import org.junit.Test; + +import java.util.Collections; +import java.util.List; + +@NoHttpd +public class CheckIT extends AbstractDaemonTest { + // Most types of tests belong in ConsistencyCheckerTest; these mostly just + // test paths outside of ConsistencyChecker, like API wiring. + @Test + public void currentPatchSetMissing() throws Exception { + PushOneCommit.Result r = createChange(); + Change c = getChange(r); + db.patchSets().deleteKeys(Collections.singleton(c.currentPatchSetId())); + + List problems = gApi.changes() + .id(r.getChangeId()) + .check() + .problems; + assertThat(problems).hasSize(1); + assertThat(problems.get(0).message) + .isEqualTo("Current patch set 1 not found"); + } + + @Test + public void fixReturnsUpdatedValue() throws Exception { + PushOneCommit.Result r = createChange(); + gApi.changes() + .id(r.getChangeId()) + .revision(r.getCommit().name()) + .review(ReviewInput.approve()); + gApi.changes() + .id(r.getChangeId()) + .revision(r.getCommit().name()) + .submit(); + + Change c = getChange(r); + c.setStatus(Change.Status.NEW); + db.changes().update(Collections.singleton(c)); + + ChangeInfo info = gApi.changes() + .id(r.getChangeId()) + .check(); + assertThat(info.problems).hasSize(1); + assertThat(info.problems.get(0).status).isNull(); + assertThat(info.status).isEqualTo(ChangeStatus.NEW); + + info = gApi.changes() + .id(r.getChangeId()) + .check(new FixInput()); + assertThat(info.problems).hasSize(1); + assertThat(info.problems.get(0).status).isEqualTo(ProblemInfo.Status.FIXED); + assertThat(info.status).isEqualTo(ChangeStatus.MERGED); + } + + private Change getChange(PushOneCommit.Result r) throws Exception { + return db.changes().get(new Change.Id( + gApi.changes().id(r.getChangeId()).get()._number)); + } +} diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 6dd6b0607b..78c95b9352 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -81,9 +81,9 @@ public interface ChangeApi { ChangeInfo get(EnumSet options) throws RestApiException; - /** {@code get} with {@link ListChangesOption} set to ALL. */ + /** {@code get} with {@link ListChangesOption} set to all except CHECK. */ ChangeInfo get() throws RestApiException; - /** {@code get} with {@link ListChangesOption} set to NONE. */ + /** {@code get} with {@link ListChangesOption} set to none. */ ChangeInfo info() throws RestApiException; /** @@ -98,6 +98,9 @@ public interface ChangeApi { */ Set getHashtags() throws RestApiException; + ChangeInfo check() throws RestApiException; + ChangeInfo check(FixInput fix) throws RestApiException; + /** * A default implementation which allows source compatibility * when adding new methods to the interface. @@ -197,5 +200,15 @@ public interface ChangeApi { public Set getHashtags() throws RestApiException { throw new NotImplementedException(); } + + @Override + public ChangeInfo check() throws RestApiException { + throw new NotImplementedException(); + } + + @Override + public ChangeInfo check(FixInput fix) throws RestApiException { + throw new NotImplementedException(); + } } } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java new file mode 100644 index 0000000000..e87be82ee5 --- /dev/null +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java @@ -0,0 +1,18 @@ +// Copyright (C) 2014 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.extensions.api.changes; + +public class FixInput { +} diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java index 5f57d18038..8fd907e1df 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java @@ -16,6 +16,7 @@ package com.google.gerrit.extensions.common; import java.sql.Timestamp; import java.util.Collection; +import java.util.List; import java.util.Map; public class ChangeInfo { @@ -50,4 +51,6 @@ public class ChangeInfo { public String currentRevision; public Map revisions; public Boolean _moreChanges; + + public List problems; } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ListChangesOption.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ListChangesOption.java index f9f8b62b19..dd3f07589d 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ListChangesOption.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ListChangesOption.java @@ -52,7 +52,10 @@ public enum ListChangesOption { DOWNLOAD_COMMANDS(13), /** Include patch set weblinks. */ - WEB_LINKS(14); + WEB_LINKS(14), + + /** Include consistency check results. */ + CHECK(15); private final int value; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CheckResult.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java similarity index 74% rename from gerrit-server/src/main/java/com/google/gerrit/server/change/CheckResult.java rename to gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java index 66c17544aa..369bcda892 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CheckResult.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ProblemInfo.java @@ -12,13 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.gerrit.server.change; +package com.google.gerrit.extensions.common; -import com.google.gerrit.extensions.common.ChangeInfo; +public class ProblemInfo { + public static enum Status { + FIXED, FIX_FAILED; + } -import java.util.List; - -public class CheckResult { - public ChangeInfo change; - public List messages; + public String message; + public Status status; + public String outcome; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index fcd669772c..9dcf97b881 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -19,6 +19,7 @@ import com.google.gerrit.extensions.api.changes.AbandonInput; import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.ChangeApi; import com.google.gerrit.extensions.api.changes.Changes; +import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.api.changes.HashtagsInput; import com.google.gerrit.extensions.api.changes.RestoreInput; import com.google.gerrit.extensions.api.changes.RevertInput; @@ -30,6 +31,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.change.Abandon; import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.change.ChangeResource; +import com.google.gerrit.server.change.Check; import com.google.gerrit.server.change.GetHashtags; import com.google.gerrit.server.change.GetTopic; import com.google.gerrit.server.change.PostHashtags; @@ -65,6 +67,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi { private final Provider changeJson; private final PostHashtags postHashtags; private final GetHashtags getHashtags; + private final Check check; @Inject ChangeApiImpl(Changes changeApi, @@ -79,6 +82,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi { Provider changeJson, PostHashtags postHashtags, GetHashtags getHashtags, + Check check, @Assisted ChangeResource change) { this.changeApi = changeApi; this.revert = revert; @@ -92,6 +96,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi { this.changeJson = changeJson; this.postHashtags = postHashtags; this.getHashtags = getHashtags; + this.check = check; this.change = change; } @@ -206,7 +211,7 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi { @Override public ChangeInfo get() throws RestApiException { - return get(EnumSet.allOf(ListChangesOption.class)); + return get(EnumSet.complementOf(EnumSet.of(ListChangesOption.CHECK))); } @Override @@ -231,4 +236,22 @@ class ChangeApiImpl extends ChangeApi.NotImplemented implements ChangeApi { throw new RestApiException("Cannot get hashtags", e); } } + + @Override + public ChangeInfo check() throws RestApiException { + try { + return check.apply(change).value(); + } catch (OrmException e) { + throw new RestApiException("Cannot check change", e); + } + } + + @Override + public ChangeInfo check(FixInput fix) throws RestApiException { + try { + return check.apply(change, fix).value(); + } catch (OrmException e) { + throw new RestApiException("Cannot check change", e); + } + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 8b1fd5f733..cdca93d982 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.change; import static com.google.gerrit.extensions.common.ListChangesOption.ALL_COMMITS; import static com.google.gerrit.extensions.common.ListChangesOption.ALL_FILES; import static com.google.gerrit.extensions.common.ListChangesOption.ALL_REVISIONS; +import static com.google.gerrit.extensions.common.ListChangesOption.CHECK; import static com.google.gerrit.extensions.common.ListChangesOption.CURRENT_ACTIONS; import static com.google.gerrit.extensions.common.ListChangesOption.CURRENT_COMMIT; import static com.google.gerrit.extensions.common.ListChangesOption.CURRENT_FILES; @@ -55,6 +56,7 @@ import com.google.gerrit.common.data.LabelValue; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.SubmitRecord; +import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ActionInfo; import com.google.gerrit.extensions.common.ApprovalInfo; @@ -65,6 +67,7 @@ import com.google.gerrit.extensions.common.FetchInfo; import com.google.gerrit.extensions.common.GitPerson; import com.google.gerrit.extensions.common.LabelInfo; import com.google.gerrit.extensions.common.ListChangesOption; +import com.google.gerrit.extensions.common.ProblemInfo; import com.google.gerrit.extensions.common.RevisionInfo; import com.google.gerrit.extensions.common.WebLinkInfo; import com.google.gerrit.extensions.config.DownloadCommand; @@ -141,8 +144,10 @@ public class ChangeJson { private final EnumSet options; private final ChangeMessagesUtil cmUtil; private final PatchLineCommentsUtil plcUtil; + private final Provider checkerProvider; private AccountLoader accountLoader; + private FixInput fix; @Inject ChangeJson( @@ -161,7 +166,8 @@ public class ChangeJson { Revisions revisions, WebLinks webLinks, ChangeMessagesUtil cmUtil, - PatchLineCommentsUtil plcUtil) { + PatchLineCommentsUtil plcUtil, + Provider checkerProvider) { this.db = db; this.labelNormalizer = ln; this.userProvider = user; @@ -178,6 +184,7 @@ public class ChangeJson { this.webLinks = webLinks; this.cmUtil = cmUtil; this.plcUtil = plcUtil; + this.checkerProvider = checkerProvider; options = EnumSet.noneOf(ListChangesOption.class); } @@ -191,6 +198,11 @@ public class ChangeJson { return this; } + public ChangeJson fix(FixInput fix) { + this.fix = fix; + return this; + } + public ChangeInfo format(ChangeResource rsrc) throws OrmException { return format(changeDataFactory.create(db.get(), rsrc.getControl())); } @@ -200,7 +212,16 @@ public class ChangeJson { } public ChangeInfo format(Change.Id id) throws OrmException { - return format(changeDataFactory.create(db.get(), id)); + Change c; + try { + c = db.get().changes().get(id); + } catch (OrmException e) { + if (!has(CHECK)) { + throw e; + } + return checkOnly(changeDataFactory.create(db.get(), id)); + } + return format(changeDataFactory.create(db.get(), c)); } public ChangeInfo format(ChangeData cd) throws OrmException { @@ -209,14 +230,21 @@ public class ChangeJson { private ChangeInfo format(ChangeData cd, Optional limitToPsId) throws OrmException { - accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); - Set reviewed = Sets.newHashSet(); - if (has(REVIEWED)) { - reviewed = loadReviewed(Collections.singleton(cd)); + try { + accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); + Set reviewed = Sets.newHashSet(); + if (has(REVIEWED)) { + reviewed = loadReviewed(Collections.singleton(cd)); + } + ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId); + accountLoader.fill(); + return res; + } catch (OrmException | RuntimeException e) { + if (!has(CHECK)) { + throw e; + } + return checkOnly(cd); } - ChangeInfo res = toChangeInfo(cd, reviewed, limitToPsId); - accountLoader.fill(); - return res; } public ChangeInfo format(RevisionResource rsrc) throws OrmException { @@ -261,10 +289,14 @@ public class ChangeJson { if (i == null) { try { i = toChangeInfo(cd, reviewed, Optional. absent()); - } catch (OrmException e) { - log.warn( - "Omitting corrupt change " + cd.getId() + " from results", e); - continue; + } catch (OrmException | RuntimeException e) { + if (has(CHECK)) { + i = checkOnly(cd); + } else { + log.warn( + "Omitting corrupt change " + cd.getId() + " from results", e); + continue; + } } out.put(cd.getId(), i); } @@ -273,11 +305,49 @@ public class ChangeJson { return info; } + private ChangeInfo checkOnly(ChangeData cd) { + ConsistencyChecker.Result result = checkerProvider.get().check(cd, fix); + ChangeInfo info; + Change c = result.change(); + if (c != null) { + info = new ChangeInfo(); + info.project = c.getProject().get(); + info.branch = c.getDest().getShortName(); + info.topic = c.getTopic(); + info.changeId = c.getKey().get(); + info.subject = c.getSubject(); + info.status = c.getStatus().asChangeStatus(); + info.owner = new AccountInfo(c.getOwner().get()); + info.created = c.getCreatedOn(); + info.updated = c.getLastUpdatedOn(); + info._number = c.getId().get(); + info.problems = result.problems(); + finish(info); + } else { + info = new ChangeInfo(); + info._number = result.id().get(); + info.problems = result.problems(); + } + return info; + } + private ChangeInfo toChangeInfo(ChangeData cd, Set reviewed, Optional limitToPsId) throws OrmException { - ChangeControl ctl = cd.changeControl().forUser(userProvider.get()); ChangeInfo out = new ChangeInfo(); + + if (has(CHECK)) { + out.problems = checkerProvider.get().check(cd.change(), fix).problems(); + // If any problems were fixed, the ChangeData needs to be reloaded. + for (ProblemInfo p : out.problems) { + if (p.status == ProblemInfo.Status.FIXED) { + cd = changeDataFactory.create(cd.db(), cd.getId()); + break; + } + } + } + Change in = cd.change(); + ChangeControl ctl = cd.changeControl().forUser(userProvider.get()); out.project = in.getProject().get(); out.branch = in.getDest().getShortName(); out.topic = in.getTopic(); @@ -355,6 +425,7 @@ public class ChangeJson { out.actions.put(descr.getId(), new ActionInfo(descr)); } } + return out; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java index cb6623146c..210eeab7dd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Check.java @@ -14,63 +14,41 @@ package com.google.gerrit.server.change; -import com.google.gerrit.extensions.common.AccountInfo; +import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.ListChangesOption; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; -import com.google.inject.Provider; -import com.google.inject.Singleton; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -@Singleton -public class Check implements RestReadView { - private static final Logger log = LoggerFactory.getLogger(Check.class); - - private final Provider checkerProvider; +public class Check implements RestReadView, + RestModifyView { private final ChangeJson json; @Inject - Check(Provider checkerProvider, - ChangeJson json) { - this.checkerProvider = checkerProvider; + Check(ChangeJson json) { this.json = json; + json.addOption(ListChangesOption.CHECK); } @Override - public CheckResult apply(ChangeResource rsrc) { - CheckResult result = new CheckResult(); - result.messages = checkerProvider.get().check(rsrc.getChange()); - try { - result.change = json.format(rsrc); - } catch (OrmException e) { - // Even with no options there are a surprising number of dependencies in - // ChangeJson. Fall back to a very basic implementation with no - // dependencies if this fails. - String msg = "Error rendering final ChangeInfo"; - log.warn(msg, e); - result.messages.add(msg); - result.change = basicChangeInfo(rsrc.getChange()); - } - return result; + public Response apply(ChangeResource rsrc) throws OrmException { + return GetChange.cache(json.format(rsrc)); } - private static ChangeInfo basicChangeInfo(Change c) { - ChangeInfo info = new ChangeInfo(); - info.project = c.getProject().get(); - info.branch = c.getDest().getShortName(); - info.topic = c.getTopic(); - info.changeId = c.getKey().get(); - info.subject = c.getSubject(); - info.status = c.getStatus().asChangeStatus(); - info.owner = new AccountInfo(c.getOwner().get()); - info.created = c.getCreatedOn(); - info.updated = c.getLastUpdatedOn(); - info._number = c.getId().get(); - ChangeJson.finish(info); - return info; + @Override + public Response apply(ChangeResource rsrc, FixInput input) + throws AuthException, OrmException { + ChangeControl ctl = rsrc.getControl(); + if (!ctl.isOwner() + && !ctl.getProjectControl().isOwner() + && !ctl.getCurrentUser().getCapabilities().canAdministrateServer()) { + throw new AuthException("Not owner"); + } + return GetChange.cache(json.fix(input).format(rsrc)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index e5c2ad8967..7197269833 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -14,16 +14,23 @@ package com.google.gerrit.server.change; +import com.google.auto.value.AutoValue; import com.google.common.base.Function; import com.google.common.collect.Collections2; import com.google.common.collect.Multimap; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.Ordering; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.api.changes.FixInput; +import com.google.gerrit.extensions.common.ProblemInfo; +import com.google.gerrit.extensions.common.ProblemInfo.Status; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -55,9 +62,28 @@ public class ConsistencyChecker { private static final Logger log = LoggerFactory.getLogger(ConsistencyChecker.class); + @AutoValue + public static abstract class Result { + private static Result create(Change.Id id, List problems) { + return new AutoValue_ConsistencyChecker_Result(id, null, problems); + } + + private static Result create(Change c, List problems) { + return new AutoValue_ConsistencyChecker_Result(c.getId(), c, problems); + } + + public abstract Change.Id id(); + + @Nullable + public abstract Change change(); + + public abstract List problems(); + } + private final Provider db; private final GitRepositoryManager repoManager; + private FixInput fix; private Change change; private Repository repo; private RevWalk rw; @@ -65,7 +91,7 @@ public class ConsistencyChecker { private PatchSet currPs; private RevCommit currPsCommit; - private List messages; + private List problems; @Inject ConsistencyChecker(Provider db, @@ -79,15 +105,34 @@ public class ConsistencyChecker { change = null; repo = null; rw = null; - messages = new ArrayList<>(); + problems = new ArrayList<>(); } - public List check(Change c) { + public Result check(ChangeData cd) { + return check(cd, null); + } + + public Result check(ChangeData cd, @Nullable FixInput f) { reset(); + try { + return check(cd.change(), f); + } catch (OrmException e) { + error("Error looking up change", e); + return Result.create(cd.getId(), problems); + } + } + + public Result check(Change c) { + return check(c, null); + } + + public Result check(Change c, @Nullable FixInput f) { + reset(); + fix = f; change = c; try { checkImpl(); - return messages; + return Result.create(c, problems); } finally { if (rw != null) { rw.release(); @@ -115,7 +160,7 @@ public class ConsistencyChecker { private void checkOwner() { try { if (db.get().accounts().get(change.getOwner()) == null) { - messages.add("Missing change owner: " + change.getOwner()); + problem("Missing change owner: " + change.getOwner()); } } catch (OrmException e) { error("Failed to look up owner", e); @@ -127,7 +172,7 @@ public class ConsistencyChecker { PatchSet.Id psId = change.currentPatchSetId(); currPs = db.get().patchSets().get(psId); if (currPs == null) { - messages.add(String.format("Current patch set %d not found", psId.get())); + problem(String.format("Current patch set %d not found", psId.get())); } } catch (OrmException e) { error("Failed to look up current patch set", e); @@ -189,7 +234,7 @@ public class ConsistencyChecker { for (Map.Entry> e : bySha.asMap().entrySet()) { if (e.getValue().size() > 1) { - messages.add(String.format("Multiple patch sets pointing to %s: %s", + problem(String.format("Multiple patch sets pointing to %s: %s", e.getKey().name(), Collections2.transform(e.getValue(), toPsId))); } @@ -204,11 +249,11 @@ public class ConsistencyChecker { try { dest = repo.getRef(refName); } catch (IOException e) { - messages.add("Failed to look up destination ref: " + refName); + problem("Failed to look up destination ref: " + refName); return; } if (dest == null) { - messages.add("Destination ref not found (may be new branch): " + problem("Destination ref not found (may be new branch): " + change.getDest().get()); return; } @@ -221,38 +266,67 @@ public class ConsistencyChecker { try { merged = rw.isMergedInto(currPsCommit, tip); } catch (IOException e) { - messages.add("Error checking whether patch set " + currPs.getId().get() + problem("Error checking whether patch set " + currPs.getId().get() + " is merged"); return; } if (merged && change.getStatus() != Change.Status.MERGED) { - messages.add(String.format("Patch set %d (%s) is merged into destination" - + " ref %s (%s), but change status is %s", currPs.getId().get(), - currPsCommit.name(), refName, tip.name(), change.getStatus())); - // TODO(dborowitz): Just fix it. + ProblemInfo p = problem(String.format( + "Patch set %d (%s) is merged into destination ref %s (%s), but change" + + " status is %s", currPs.getId().get(), currPsCommit.name(), + refName, tip.name(), change.getStatus())); + if (fix != null) { + fixMerged(p); + } } else if (!merged && change.getStatus() == Change.Status.MERGED) { - messages.add(String.format("Patch set %d (%s) is not merged into" + problem(String.format("Patch set %d (%s) is not merged into" + " destination ref %s (%s), but change status is %s", currPs.getId().get(), currPsCommit.name(), refName, tip.name(), change.getStatus())); } } + private void fixMerged(ProblemInfo p) { + try { + change = db.get().changes().atomicUpdate(change.getId(), + new AtomicUpdate() { + @Override + public Change update(Change c) { + c.setStatus(Change.Status.MERGED); + return c; + } + }); + p.status = Status.FIXED; + p.outcome = "Marked change as merged"; + } catch (OrmException e) { + log.warn("Error marking " + change.getId() + "as merged", e); + p.status = Status.FIX_FAILED; + p.outcome = "Error updating status to merged"; + } + } + private RevCommit parseCommit(ObjectId objId, String desc) { try { return rw.parseCommit(objId); } catch (MissingObjectException e) { - messages.add(String.format("Object missing: %s: %s", desc, objId.name())); + problem(String.format("Object missing: %s: %s", desc, objId.name())); } catch (IncorrectObjectTypeException e) { - messages.add(String.format("Not a commit: %s: %s", desc, objId.name())); + problem(String.format("Not a commit: %s: %s", desc, objId.name())); } catch (IOException e) { - messages.add(String.format("Failed to look up: %s: %s", desc, objId.name())); + problem(String.format("Failed to look up: %s: %s", desc, objId.name())); } return null; } + private ProblemInfo problem(String msg) { + ProblemInfo p = new ProblemInfo(); + p.message = msg; + problems.add(p); + return p; + } + private boolean error(String msg, Throwable t) { - messages.add(msg); + problem(msg); // TODO(dborowitz): Expose stack trace to administrators. log.warn("Error in consistency check of change " + change.getId(), t); return false; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetChange.java index d3bbb13692..f3673c9cc1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetChange.java @@ -53,7 +53,7 @@ public class GetChange implements RestReadView { return cache(json.format(rsrc)); } - private Response cache(ChangeInfo res) { + static Response cache(ChangeInfo res) { return Response.ok(res) .caching(CacheControl.PRIVATE(0, TimeUnit.SECONDS).setMustRevalidate()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index d2d3db342e..4cf7692baf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -53,6 +53,7 @@ public class Module extends RestApiModule { get(CHANGE_KIND, "in").to(IncludedIn.class); get(CHANGE_KIND, "hashtags").to(GetHashtags.class); get(CHANGE_KIND, "check").to(Check.class); + post(CHANGE_KIND, "check").to(Check.class); put(CHANGE_KIND, "topic").to(PutTopic.class); delete(CHANGE_KIND, "topic").to(PutTopic.class); delete(CHANGE_KIND).to(DeleteDraftChange.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index d45a58eb80..0f1a7bbc78 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -594,6 +594,9 @@ public class ChangeData { mergeable = true; } else { PatchSet ps = currentPatchSet(); + if (ps == null) { + throw new OrmException("Missing patch set for mergeability check"); + } Repository repo = null; try { repo = repoManager.openRepository(c.getProject()); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java index 3104b69fd2..51aa283c45 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/ConsistencyCheckerTest.java @@ -20,7 +20,11 @@ import static com.google.gerrit.testutil.TestChanges.newChange; import static com.google.gerrit.testutil.TestChanges.newPatchSet; import static java.util.Collections.singleton; +import com.google.common.base.Function; +import com.google.common.collect.Lists; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.api.changes.FixInput; +import com.google.gerrit.extensions.common.ProblemInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; @@ -40,6 +44,8 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; +import java.util.List; + public class ConsistencyCheckerTest { private InMemoryDatabase schemaFactory; private ReviewDb db; @@ -90,7 +96,7 @@ public class ConsistencyCheckerTest { PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId); db.patchSets().insert(singleton(ps2)); - assertThat(checker.check(c)).isEmpty(); + assertProblems(c); } @Test @@ -110,7 +116,7 @@ public class ConsistencyCheckerTest { db.patchSets().insert(singleton(ps2)); repo.branch(c.getDest().get()).update(commit2); - assertThat(checker.check(c)).isEmpty(); + assertProblems(c); } @Test @@ -122,7 +128,7 @@ public class ConsistencyCheckerTest { PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId); db.patchSets().insert(singleton(ps)); - assertThat(checker.check(c)).containsExactly("Missing change owner: 2"); + assertProblems(c, "Missing change owner: 2"); } @Test @@ -132,8 +138,7 @@ public class ConsistencyCheckerTest { PatchSet ps = newPatchSet(c.currentPatchSetId(), ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId); db.patchSets().insert(singleton(ps)); - assertThat(checker.check(c)) - .containsExactly("Destination repository not found: otherproject"); + assertProblems(c, "Destination repository not found: otherproject"); } @Test @@ -153,7 +158,7 @@ public class ConsistencyCheckerTest { PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId); db.patchSets().insert(singleton(ps2)); - assertThat(checker.check(c)).containsExactly( + assertProblems(c, "Invalid revision on patch set 1:" + " fooooooooooooooooooooooooooooooooooooooo"); } @@ -166,7 +171,7 @@ public class ConsistencyCheckerTest { ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), userId); db.patchSets().insert(singleton(ps)); - assertThat(checker.check(c)).containsExactly( + assertProblems(c, "Object missing: patch set 1: deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); } @@ -174,8 +179,7 @@ public class ConsistencyCheckerTest { public void currentPatchSetMissing() throws Exception { Change c = newChange(project, userId); db.changes().insert(singleton(c)); - assertThat(checker.check(c)) - .containsExactly("Current patch set 1 not found"); + assertProblems(c, "Current patch set 1 not found"); } @Test @@ -191,8 +195,8 @@ public class ConsistencyCheckerTest { PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit1, userId); db.patchSets().insert(singleton(ps2)); - assertThat(checker.check(c)).containsExactly("Multiple patch sets pointing to " - + commit1.name() + ": [1, 2]"); + assertProblems(c, + "Multiple patch sets pointing to " + commit1.name() + ": [1, 2]"); } @Test @@ -206,8 +210,7 @@ public class ConsistencyCheckerTest { PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId); db.patchSets().insert(singleton(ps)); - assertThat(checker.check(c)).containsExactly( - "Destination ref not found (may be new branch): master"); + assertProblems(c, "Destination ref not found (may be new branch): master"); } @Test @@ -220,7 +223,7 @@ public class ConsistencyCheckerTest { PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId); db.patchSets().insert(singleton(ps)); - assertThat(checker.check(c)).containsExactly( + assertProblems(c, "Patch set 1 (" + commit.name() + ") is not merged into destination ref" + " master (" + tip.name() + "), but change status is MERGED"); } @@ -235,8 +238,42 @@ public class ConsistencyCheckerTest { db.patchSets().insert(singleton(ps)); repo.branch(c.getDest().get()).update(commit); - assertThat(checker.check(c)).containsExactly( + assertProblems(c, "Patch set 1 (" + commit.name() + ") is merged into destination ref" + " master (" + commit.name() + "), but change status is NEW"); } + + @Test + public void newChangeIsMergedWithFix() throws Exception { + Change c = newChange(project, userId); + db.changes().insert(singleton(c)); + RevCommit commit = repo.branch(c.currentPatchSetId().toRefName()).commit() + .parent(tip).create(); + PatchSet ps = newPatchSet(c.currentPatchSetId(), commit, userId); + db.patchSets().insert(singleton(ps)); + repo.branch(c.getDest().get()).update(commit); + + List problems = checker.check(c, new FixInput()).problems(); + assertThat(problems).hasSize(1); + ProblemInfo p = problems.get(0); + assertThat(p.message).isEqualTo( + "Patch set 1 (" + commit.name() + ") is merged into destination ref" + + " master (" + commit.name() + "), but change status is NEW"); + assertThat(p.status).isEqualTo(ProblemInfo.Status.FIXED); + assertThat(p.outcome).isEqualTo("Marked change as merged"); + + c = db.changes().get(c.getId()); + assertThat(c.getStatus()).isEqualTo(Change.Status.MERGED); + assertProblems(c); + } + + private void assertProblems(Change c, String... expected) { + assertThat(Lists.transform(checker.check(c).problems(), + new Function() { + @Override + public String apply(ProblemInfo in) { + return in.message; + } + })).containsExactly((Object[]) expected); + } }