OnSubmitValidationListener: Don't expose Repository
If we give extensions a full Repository, it's possible to use it in an incorrect way. For example, a RevWalk created with `new RevWalk(repo)` will not be able to read the ObjectIds returned by `getCommands()`. Instead, expose only a RevWalk and a getRef method that calls into the ChainedReceiveCommands. This is more limiting, because it can't do everything a Repository can do, but it is impossible to use in this incorrect way. The only known existing OnSubmitValidationListener is in the git-numberer plugin[1], which will be able to do what it needs with this restricted interface. [1] https://chromium-review.googlesource.com/q/project:infra%252Fgerrit-plugins%252Fgit-numberer Change-Id: I4a20ea34e49e30714e269b46835b55c43d7a121e
This commit is contained in:
@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.rest.change;
|
||||
import static com.google.common.collect.Iterables.getOnlyElement;
|
||||
import static com.google.common.truth.Truth.assertThat;
|
||||
import static com.google.common.truth.Truth.assert_;
|
||||
import static com.google.common.truth.Truth8.assertThat;
|
||||
import static com.google.common.truth.TruthJUnit.assume;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
|
||||
import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
|
||||
@@ -95,6 +96,7 @@ import org.eclipse.jgit.lib.Ref;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
import org.eclipse.jgit.revwalk.RevCommit;
|
||||
import org.eclipse.jgit.revwalk.RevWalk;
|
||||
import org.eclipse.jgit.transport.ReceiveCommand;
|
||||
import org.junit.After;
|
||||
import org.junit.Before;
|
||||
import org.junit.Test;
|
||||
@@ -756,11 +758,16 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
|
||||
new OnSubmitValidationListener() {
|
||||
@Override
|
||||
public void preBranchUpdate(Arguments args) throws ValidationException {
|
||||
assertThat(args.getCommands().keySet()).contains("refs/heads/master");
|
||||
try (RevWalk rw = args.newRevWalk()) {
|
||||
rw.parseBody(rw.parseCommit(args.getCommands().get("refs/heads/master").getNewId()));
|
||||
String master = "refs/heads/master";
|
||||
assertThat(args.getCommands()).containsKey(master);
|
||||
ReceiveCommand cmd = args.getCommands().get(master);
|
||||
ObjectId newMasterId = cmd.getNewId();
|
||||
try (Repository repo = repoManager.openRepository(project)) {
|
||||
assertThat(repo.exactRef(master).getObjectId()).isEqualTo(cmd.getOldId());
|
||||
assertThat(args.getRef(master)).hasValue(newMasterId);
|
||||
args.getRevWalk().parseBody(args.getRevWalk().parseCommit(newMasterId));
|
||||
} catch (IOException e) {
|
||||
assertThat(e).isNull();
|
||||
throw new AssertionError("failed checking new ref value", e);
|
||||
}
|
||||
projectsCalled.add(args.getProject().get());
|
||||
if (projectsCalled.size() == 2) {
|
||||
|
@@ -11,15 +11,20 @@
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package com.google.gerrit.server.git.validators;
|
||||
|
||||
import static com.google.common.base.Preconditions.checkNotNull;
|
||||
|
||||
import com.google.common.collect.ImmutableMap;
|
||||
import com.google.gerrit.extensions.annotations.ExtensionPoint;
|
||||
import com.google.gerrit.reviewdb.client.Project;
|
||||
import com.google.gerrit.reviewdb.client.Project.NameKey;
|
||||
import com.google.gerrit.server.git.RefCache;
|
||||
import com.google.gerrit.server.update.ChainedReceiveCommands;
|
||||
import com.google.gerrit.server.validators.ValidationException;
|
||||
import java.util.Map;
|
||||
import org.eclipse.jgit.lib.ObjectReader;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
import java.io.IOException;
|
||||
import java.util.Optional;
|
||||
import org.eclipse.jgit.lib.ObjectId;
|
||||
import org.eclipse.jgit.revwalk.RevWalk;
|
||||
import org.eclipse.jgit.transport.ReceiveCommand;
|
||||
|
||||
@@ -37,41 +42,58 @@ import org.eclipse.jgit.transport.ReceiveCommand;
|
||||
public interface OnSubmitValidationListener {
|
||||
class Arguments {
|
||||
private Project.NameKey project;
|
||||
private Repository repository;
|
||||
private ObjectReader objectReader;
|
||||
private Map<String, ReceiveCommand> commands;
|
||||
private RevWalk rw;
|
||||
private ImmutableMap<String, ReceiveCommand> commands;
|
||||
private RefCache refs;
|
||||
|
||||
public Arguments(
|
||||
NameKey project,
|
||||
Repository repository,
|
||||
ObjectReader objectReader,
|
||||
Map<String, ReceiveCommand> commands) {
|
||||
this.project = project;
|
||||
this.repository = repository;
|
||||
this.objectReader = objectReader;
|
||||
this.commands = commands;
|
||||
/**
|
||||
* @param project project.
|
||||
* @param rw revwalk that can read unflushed objects from {@code refs}.
|
||||
* @param commands commands to be executed.
|
||||
*/
|
||||
Arguments(Project.NameKey project, RevWalk rw, ChainedReceiveCommands commands) {
|
||||
this.project = checkNotNull(project);
|
||||
this.rw = checkNotNull(rw);
|
||||
this.refs = checkNotNull(commands);
|
||||
this.commands = ImmutableMap.copyOf(commands.getCommands());
|
||||
}
|
||||
|
||||
/** Get the project name for this operation. */
|
||||
public Project.NameKey getProject() {
|
||||
return project;
|
||||
}
|
||||
|
||||
/** @return a read only repository */
|
||||
public Repository getRepository() {
|
||||
return repository;
|
||||
}
|
||||
|
||||
public RevWalk newRevWalk() {
|
||||
return new RevWalk(objectReader);
|
||||
/**
|
||||
* Get a revwalk for this operation.
|
||||
*
|
||||
* <p>This instance is able to read all objects mentioned in {@link #getCommands()} and {@link
|
||||
* #getRef(String)}.
|
||||
*
|
||||
* @return open revwalk.
|
||||
*/
|
||||
public RevWalk getRevWalk() {
|
||||
return rw;
|
||||
}
|
||||
|
||||
/**
|
||||
* @return a map from ref to op on it covering all ref ops to be performed on this repository as
|
||||
* part of ongoing submit operation.
|
||||
* @return a map from ref to commands covering all ref operations to be performed on this
|
||||
* repository as part of the ongoing submit operation.
|
||||
*/
|
||||
public Map<String, ReceiveCommand> getCommands() {
|
||||
public ImmutableMap<String, ReceiveCommand> getCommands() {
|
||||
return commands;
|
||||
}
|
||||
|
||||
/**
|
||||
* Get a ref from the repository.
|
||||
*
|
||||
* @param name ref name; can be any ref, not just the ones mentioned in {@link #getCommands()}.
|
||||
* @return latest value of a ref in the repository, as if all commands from {@link
|
||||
* #getCommands()} had already been applied.
|
||||
* @throws IOException if an error occurred reading the ref.
|
||||
*/
|
||||
public Optional<ObjectId> getRef(String name) throws IOException {
|
||||
return refs.get(name);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
@@ -11,18 +11,18 @@
|
||||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
// See the License for the specific language governing permissions and
|
||||
// limitations under the License.
|
||||
|
||||
package com.google.gerrit.server.git.validators;
|
||||
|
||||
import com.google.gerrit.extensions.registration.DynamicSet;
|
||||
import com.google.gerrit.reviewdb.client.Project;
|
||||
import com.google.gerrit.server.git.IntegrationException;
|
||||
import com.google.gerrit.server.git.validators.OnSubmitValidationListener.Arguments;
|
||||
import com.google.gerrit.server.update.ChainedReceiveCommands;
|
||||
import com.google.gerrit.server.validators.ValidationException;
|
||||
import com.google.inject.Inject;
|
||||
import java.util.Map;
|
||||
import org.eclipse.jgit.lib.ObjectReader;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
import org.eclipse.jgit.transport.ReceiveCommand;
|
||||
import org.eclipse.jgit.revwalk.RevWalk;
|
||||
|
||||
public class OnSubmitValidators {
|
||||
public interface Factory {
|
||||
@@ -37,14 +37,12 @@ public class OnSubmitValidators {
|
||||
}
|
||||
|
||||
public void validate(
|
||||
Project.NameKey project,
|
||||
Repository repo,
|
||||
ObjectReader objectReader,
|
||||
Map<String, ReceiveCommand> commands)
|
||||
Project.NameKey project, ObjectReader objectReader, ChainedReceiveCommands commands)
|
||||
throws IntegrationException {
|
||||
try {
|
||||
for (OnSubmitValidationListener listener : this.listeners) {
|
||||
listener.preBranchUpdate(new Arguments(project, repo, objectReader, commands));
|
||||
try (RevWalk rw = new RevWalk(objectReader)) {
|
||||
Arguments args = new Arguments(project, rw, commands);
|
||||
for (OnSubmitValidationListener listener : listeners) {
|
||||
listener.preBranchUpdate(args);
|
||||
}
|
||||
} catch (ValidationException e) {
|
||||
throw new IntegrationException(e.getMessage());
|
||||
|
@@ -313,16 +313,11 @@ class NoteDbBatchUpdate extends BatchUpdate {
|
||||
}
|
||||
|
||||
if (onSubmitValidators != null && commands != null && !commands.isEmpty()) {
|
||||
// Validation of refs has to take place here and not at the beginning
|
||||
// executeRefUpdates. Otherwise failing validation in a second
|
||||
// BatchUpdate object will happen *after* first object's
|
||||
// executeRefUpdates has finished, hence after first repo's refs have
|
||||
// been updated, which is too late.
|
||||
onSubmitValidators.validate(
|
||||
project,
|
||||
new ReadOnlyRepository(getRepository()),
|
||||
ctx.getInserter().newReader(),
|
||||
commands.getCommands());
|
||||
// Validation of refs has to take place here and not at the beginning of executeRefUpdates.
|
||||
// Otherwise, failing validation in a second BatchUpdate object will happen *after* the
|
||||
// first update's executeRefUpdates has finished, hence after first repo's refs have been
|
||||
// updated, which is too late.
|
||||
onSubmitValidators.validate(project, ctx.getRevWalk().getObjectReader(), commands);
|
||||
}
|
||||
|
||||
// TODO(dborowitz): Don't flush when fusing phases.
|
||||
|
@@ -79,7 +79,6 @@ import org.eclipse.jgit.lib.BatchRefUpdate;
|
||||
import org.eclipse.jgit.lib.Config;
|
||||
import org.eclipse.jgit.lib.NullProgressMonitor;
|
||||
import org.eclipse.jgit.lib.ObjectInserter;
|
||||
import org.eclipse.jgit.lib.ObjectReader;
|
||||
import org.eclipse.jgit.lib.PersonIdent;
|
||||
import org.eclipse.jgit.lib.Repository;
|
||||
import org.eclipse.jgit.revwalk.RevWalk;
|
||||
@@ -399,14 +398,11 @@ class ReviewDbBatchUpdate extends BatchUpdate {
|
||||
}
|
||||
|
||||
if (onSubmitValidators != null && commands != null && !commands.isEmpty()) {
|
||||
try (ObjectReader reader = ctx.getInserter().newReader()) {
|
||||
// Validation of refs has to take place here and not at the beginning
|
||||
// executeRefUpdates. Otherwise failing validation in a second BatchUpdate object will
|
||||
// happen *after* first object's executeRefUpdates has finished, hence after first repo's
|
||||
// refs have been updated, which is too late.
|
||||
onSubmitValidators.validate(
|
||||
project, new ReadOnlyRepository(getRepository()), reader, commands.getCommands());
|
||||
}
|
||||
// Validation of refs has to take place here and not at the beginning of executeRefUpdates.
|
||||
// Otherwise, failing validation in a second BatchUpdate object will happen *after* the
|
||||
// first update's executeRefUpdates has finished, hence after first repo's refs have been
|
||||
// updated, which is too late.
|
||||
onSubmitValidators.validate(project, ctx.getRevWalk().getObjectReader(), commands);
|
||||
}
|
||||
|
||||
if (inserter != null) {
|
||||
|
Reference in New Issue
Block a user