Warn extension API users that underlying objects are read eagerly
Storing references to these APIs is risky because modifications via
the API instance may or may not be reflected in later getter calls,
depending on what part of the object was modified and how the
implementation works. Explicitly discourage this behavior, and change
all callers within Gerrit.
A later change may remove this recommendation after modifying all API
implementations, but that is an unknown amount of work, and there is
no harm in documenting the current behavior now.
Change-Id: I1921b619753b1d1cc60426704f68ff0318bf6fe5
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 6159ae3..6a58933 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
@@ -26,7 +26,6 @@
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
-import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -116,7 +115,9 @@
.rebase();
}
- private static Set<Account.Id> getReviewers(ChangeInfo ci) {
+ private Set<Account.Id> getReviewers(String changeId)
+ throws RestApiException {
+ ChangeInfo ci = gApi.changes().id(changeId).get();
Set<Account.Id> result = Sets.newHashSet();
for (LabelInfo li : ci.labels.values()) {
for (ApprovalInfo ai : li.all) {
@@ -132,22 +133,26 @@
PushOneCommit.Result r = createChange();
AddReviewerInput in = new AddReviewerInput();
in.reviewer = user.email;
- ChangeApi cApi = gApi.changes().id(r.getChangeId());
- cApi.addReviewer(in);
- assertEquals(ImmutableSet.of(user.id), getReviewers(cApi.get()));
+ gApi.changes()
+ .id(r.getChangeId())
+ .addReviewer(in);
+ assertEquals(ImmutableSet.of(user.id), getReviewers(r.getChangeId()));
}
@Test
public void addReviewerToClosedChange() throws GitAPIException,
IOException, RestApiException {
PushOneCommit.Result r = createChange();
- ChangeApi cApi = gApi.changes().id(r.getChangeId());
- cApi.revision(r.getCommit().name())
+ gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
.review(ReviewInput.approve());
- cApi.revision(r.getCommit().name())
+ gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
.submit();
- assertEquals(ImmutableSet.of(admin.getId()), getReviewers(cApi.get()));
+ assertEquals(ImmutableSet.of(admin.getId()), getReviewers(r.getChangeId()));
AddReviewerInput in = new AddReviewerInput();
in.reviewer = user.email;
@@ -155,7 +160,7 @@
.id(r.getChangeId())
.addReviewer(in);
assertEquals(ImmutableSet.of(admin.getId(), user.id),
- getReviewers(cApi.get()));
+ getReviewers(r.getChangeId()));
}
@Test
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index e18ac17..54e8799 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -27,7 +27,6 @@
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -264,12 +263,6 @@
.isEmpty());
}
- protected RevisionApi revision(PushOneCommit.Result r) throws Exception {
- return gApi.changes()
- .id(r.getChangeId())
- .current();
- }
-
private void merge(PushOneCommit.Result r) throws Exception {
revision(r).review(ReviewInput.approve());
revision(r).submit();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
index f636f11..983198a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
@@ -25,7 +25,6 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.extensions.api.GerritApi;
-import com.google.gerrit.extensions.api.projects.ProjectApi;
import com.google.gerrit.extensions.api.projects.ProjectInput;
import com.google.gerrit.extensions.common.InheritableBoolean;
import com.google.gerrit.extensions.common.ProjectInfo;
@@ -74,8 +73,7 @@
@Test
public void testCreateProjectApi() throws RestApiException, IOException {
final String newProjectName = "newProject";
- ProjectApi projectApi = gApi.projects().name(newProjectName).create();
- ProjectInfo p = projectApi.get();
+ ProjectInfo p = gApi.projects().name(newProjectName).create().get();
assertEquals(newProjectName, p.name);
ProjectState projectState = projectCache.get(new Project.NameKey(newProjectName));
assertNotNull(projectState);
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/Accounts.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/Accounts.java
index 749b12a..71a93d3 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/Accounts.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/accounts/Accounts.java
@@ -18,7 +18,27 @@
import com.google.gerrit.extensions.restapi.RestApiException;
public interface Accounts {
+ /**
+ * Look up an account by ID.
+ * <p>
+ * <strong>Note:</strong> This method eagerly reads the account. Methods that
+ * mutate the account do not necessarily re-read the account. Therefore, calling
+ * a getter method on an instance after calling a mutation method on that same
+ * instance is not guaranteed to reflect the mutation. It is not recommended
+ * to store references to {@code AccountApi} instances.
+ *
+ * @param id any identifier supported by the REST API, including numeric ID,
+ * email, or username.
+ * @return API for accessing the account.
+ * @throws RestApiException if an error occurred.
+ */
AccountApi id(String id) throws RestApiException;
+
+ /**
+ * Look up the account of the current in-scope user.
+ *
+ * @see #id(String)
+ */
AccountApi self() throws RestApiException;
/**
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 c3ade5f..4d509e9 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
@@ -24,8 +24,32 @@
public interface ChangeApi {
String id();
+ /**
+ * Look up the current revision for the change.
+ * <p>
+ * <strong>Note:</strong> This method eagerly reads the revision. Methods that
+ * mutate the revision do not necessarily re-read the revision. Therefore,
+ * calling a getter method on an instance after calling a mutation method on
+ * that same instance is not guaranteed to reflect the mutation. It is not
+ * recommended to store references to {@code RevisionApi} instances.
+ *
+ * @return API for accessing the revision.
+ * @throws RestApiException if an error occurred.
+ */
RevisionApi current() throws RestApiException;
+
+ /**
+ * Look up a revision of a change by number.
+ *
+ * @see #current()
+ */
RevisionApi revision(int id) throws RestApiException;
+
+ /**
+ * Look up a revision of a change by commit SHA-1.
+ *
+ * @see #current()
+ */
RevisionApi revision(String id) throws RestApiException;
void abandon() throws RestApiException;
@@ -34,7 +58,18 @@
void restore() throws RestApiException;
void restore(RestoreInput in) throws RestApiException;
+ /**
+ * Create a new change that reverts this change.
+ *
+ * @see Changes#id(int)
+ */
ChangeApi revert() throws RestApiException;
+
+ /**
+ * Create a new change that reverts this change.
+ *
+ * @see Changes#id(int)
+ */
ChangeApi revert(RevertInput in) throws RestApiException;
String topic() throws RestApiException;
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java
index 201a0bd..4084946 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java
@@ -24,10 +24,40 @@
import java.util.List;
public interface Changes {
+ /**
+ * Look up a change by numeric ID.
+ * <p>
+ * <strong>Note:</strong> This method eagerly reads the change. Methods that
+ * mutate the change do not necessarily re-read the change. Therefore, calling
+ * a getter method on an instance after calling a mutation method on that same
+ * instance is not guaranteed to reflect the mutation. It is not recommended
+ * to store references to {@code ChangeApi} instances.
+ *
+ * @param id change number.
+ * @return API for accessing the change.
+ * @throws RestApiException if an error occurred.
+ */
ChangeApi id(int id) throws RestApiException;
- ChangeApi id(String triplet) throws RestApiException;
+
+ /**
+ * Look up a change by string ID.
+ *
+ * @see #id(int)
+ * @param id any identifier supported by the REST API, including change
+ * number, Change-Id, or project~branch~Change-Id triplet.
+ * @return API for accessing the change.
+ * @throws RestApiException if an error occurred.
+ */
+ ChangeApi id(String id) throws RestApiException;
+
+ /**
+ * Look up a change by project, branch, and change ID.
+ *
+ * @see #id(int)
+ */
ChangeApi id(String project, String branch, String id)
throws RestApiException;
+
ChangeApi create(ChangeInfo in) throws RestApiException;
QueryRequest query();
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java
index d013c5d..07a48a1 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ProjectApi.java
@@ -22,6 +22,19 @@
ProjectApi create() throws RestApiException;
ProjectApi create(ProjectInput in) throws RestApiException;
ProjectInfo get();
+
+ /**
+ * Look up a branch by refname.
+ * <p>
+ * <strong>Note:</strong> This method eagerly reads the branch. Methods that
+ * mutate the branch do not necessarily re-read the branch. Therefore, calling
+ * a getter method on an instance after calling a mutation method on that same
+ * instance is not guaranteed to reflect the mutation. It is not recommended
+ * to store references to {@code BranchApi} instances.
+ *
+ * @param ref branch name, with or without "refs/heads/" prefix.
+ * @return API for accessing the branch.
+ */
BranchApi branch(String ref);
/**
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/Projects.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/Projects.java
index 9c0cfd8..736d375 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/Projects.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/Projects.java
@@ -21,6 +21,19 @@
import java.util.List;
public interface Projects {
+ /**
+ * Look up a project by name.
+ * <p>
+ * <strong>Note:</strong> This method eagerly reads the project. Methods that
+ * mutate the project do not necessarily re-read the project. Therefore,
+ * calling a getter method on an instance after calling a mutation method on
+ * that same instance is not guaranteed to reflect the mutation. It is not
+ * recommended to store references to {@code ProjectApi} instances.
+ *
+ * @param name project name.
+ * @return API for accessing the project.
+ * @throws RestApiException if an error occurred.
+ */
ProjectApi name(String name) throws RestApiException;
ListRequest list();
