Merge changes from topic 'check'
* changes:
ChangeJson: Catch RuntimeException in fallback check handlers
Support fixing corrupt changes in ConsistencyChecker
Add API method for checking changes
Check consistency with a ListChangesOption
Embed consistency check results in ChangeInfo result
ChangeData: Handle null PatchSet in isMergeable
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index db358f3..bd94789 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -300,6 +300,11 @@
* `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 @@
--
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 @@
)]}'
{
- "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 @@
|`_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 @@
|`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 ae7e94b..ec21328 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 @@
.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 0000000..db45e4c
--- /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<ProblemInfo> 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 6dd6b06..78c95b9 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 @@
ChangeInfo get(EnumSet<ListChangesOption> 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 @@
*/
Set<String> 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 Set<String> 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-server/src/main/java/com/google/gerrit/server/change/CheckResult.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java
similarity index 74%
copy from gerrit-server/src/main/java/com/google/gerrit/server/change/CheckResult.java
copy to gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/FixInput.java
index 66c1754..e87be82 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/api/changes/FixInput.java
@@ -12,13 +12,7 @@
// 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.api.changes;
-import com.google.gerrit.extensions.common.ChangeInfo;
-
-import java.util.List;
-
-public class CheckResult {
- public ChangeInfo change;
- public List<String> messages;
+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 5f57d18..8fd907e 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 @@
import java.sql.Timestamp;
import java.util.Collection;
+import java.util.List;
import java.util.Map;
public class ChangeInfo {
@@ -50,4 +51,6 @@
public String currentRevision;
public Map<String, RevisionInfo> revisions;
public Boolean _moreChanges;
+
+ public List<ProblemInfo> 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 f9f8b62..dd3f075 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 @@
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 66c1754..369bcda 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<String> 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 fcd6697..9dcf97b 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.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.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 @@
private final Provider<ChangeJson> changeJson;
private final PostHashtags postHashtags;
private final GetHashtags getHashtags;
+ private final Check check;
@Inject
ChangeApiImpl(Changes changeApi,
@@ -79,6 +82,7 @@
Provider<ChangeJson> changeJson,
PostHashtags postHashtags,
GetHashtags getHashtags,
+ Check check,
@Assisted ChangeResource change) {
this.changeApi = changeApi;
this.revert = revert;
@@ -92,6 +96,7 @@
this.changeJson = changeJson;
this.postHashtags = postHashtags;
this.getHashtags = getHashtags;
+ this.check = check;
this.change = change;
}
@@ -206,7 +211,7 @@
@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 @@
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 8b1fd5f..cdca93d 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 @@
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.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.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 @@
private final EnumSet<ListChangesOption> options;
private final ChangeMessagesUtil cmUtil;
private final PatchLineCommentsUtil plcUtil;
+ private final Provider<ConsistencyChecker> checkerProvider;
private AccountLoader accountLoader;
+ private FixInput fix;
@Inject
ChangeJson(
@@ -161,7 +166,8 @@
Revisions revisions,
WebLinks webLinks,
ChangeMessagesUtil cmUtil,
- PatchLineCommentsUtil plcUtil) {
+ PatchLineCommentsUtil plcUtil,
+ Provider<ConsistencyChecker> checkerProvider) {
this.db = db;
this.labelNormalizer = ln;
this.userProvider = user;
@@ -178,6 +184,7 @@
this.webLinks = webLinks;
this.cmUtil = cmUtil;
this.plcUtil = plcUtil;
+ this.checkerProvider = checkerProvider;
options = EnumSet.noneOf(ListChangesOption.class);
}
@@ -191,6 +198,11 @@
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 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 @@
private ChangeInfo format(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
throws OrmException {
- accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS));
- Set<Change.Id> reviewed = Sets.newHashSet();
- if (has(REVIEWED)) {
- reviewed = loadReviewed(Collections.singleton(cd));
+ try {
+ accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS));
+ Set<Change.Id> 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 @@
if (i == null) {
try {
i = toChangeInfo(cd, reviewed, Optional.<PatchSet.Id> 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 @@
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<Change.Id> reviewed,
Optional<PatchSet.Id> 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 @@
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 cb66231..210eeab 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<ChangeResource> {
- private static final Logger log = LoggerFactory.getLogger(Check.class);
-
- private final Provider<ConsistencyChecker> checkerProvider;
+public class Check implements RestReadView<ChangeResource>,
+ RestModifyView<ChangeResource, FixInput> {
private final ChangeJson json;
@Inject
- Check(Provider<ConsistencyChecker> 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<ChangeInfo> 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<ChangeInfo> 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 e5c2ad8..7197269 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 @@
private static final Logger log =
LoggerFactory.getLogger(ConsistencyChecker.class);
+ @AutoValue
+ public static abstract class Result {
+ private static Result create(Change.Id id, List<ProblemInfo> problems) {
+ return new AutoValue_ConsistencyChecker_Result(id, null, problems);
+ }
+
+ private static Result create(Change c, List<ProblemInfo> problems) {
+ return new AutoValue_ConsistencyChecker_Result(c.getId(), c, problems);
+ }
+
+ public abstract Change.Id id();
+
+ @Nullable
+ public abstract Change change();
+
+ public abstract List<ProblemInfo> problems();
+ }
+
private final Provider<ReviewDb> db;
private final GitRepositoryManager repoManager;
+ private FixInput fix;
private Change change;
private Repository repo;
private RevWalk rw;
@@ -65,7 +91,7 @@
private PatchSet currPs;
private RevCommit currPsCommit;
- private List<String> messages;
+ private List<ProblemInfo> problems;
@Inject
ConsistencyChecker(Provider<ReviewDb> db,
@@ -79,15 +105,34 @@
change = null;
repo = null;
rw = null;
- messages = new ArrayList<>();
+ problems = new ArrayList<>();
}
- public List<String> 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 @@
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 @@
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 @@
for (Map.Entry<ObjectId, Collection<PatchSet>> 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 @@
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 @@
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<Change>() {
+ @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 d3bbb13..f3673c9 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 @@
return cache(json.format(rsrc));
}
- private Response<ChangeInfo> cache(ChangeInfo res) {
+ static Response<ChangeInfo> 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 d2d3db3..4cf7692 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 @@
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 d45a58e..0f1a7bb 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 @@
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 3104b69..51aa283 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.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.Before;
import org.junit.Test;
+import java.util.List;
+
public class ConsistencyCheckerTest {
private InMemoryDatabase schemaFactory;
private ReviewDb db;
@@ -90,7 +96,7 @@
PatchSet ps2 = newPatchSet(c.currentPatchSetId(), commit2, userId);
db.patchSets().insert(singleton(ps2));
- assertThat(checker.check(c)).isEmpty();
+ assertProblems(c);
}
@Test
@@ -110,7 +116,7 @@
db.patchSets().insert(singleton(ps2));
repo.branch(c.getDest().get()).update(commit2);
- assertThat(checker.check(c)).isEmpty();
+ assertProblems(c);
}
@Test
@@ -122,7 +128,7 @@
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 @@
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 @@
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 @@
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 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 @@
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 @@
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 @@
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 @@
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<ProblemInfo> 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<ProblemInfo, String>() {
+ @Override
+ public String apply(ProblemInfo in) {
+ return in.message;
+ }
+ })).containsExactly((Object[]) expected);
+ }
}
