opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.14'

Browse Source 
* stable-2.14:
  Enable OAuth token setting when gitBasicAuthPolicy equals OAUTH
  Enable HTTP password setting based upon gitBasicAuthPolicy
  Fix file_name column size in H2AccountPatchReviewStore
  GET files?reviewed: Don't fire N+1 selects for N patch sets
  Use lower case table/column/constraint names in H2AccountPatchReviewStore
  Fix clear methods in H2AccountPatchReviewStore
  JestClientBuilder: Format with google-java-format
  ES: Extract Jest client builder into separate class
  Remove unused import and apply formatting in AbstractElasticIndex
  Fix: Default values for Elastic index.* config values

Change-Id: I9e4ff0bd290df5b65ced77c541dec3b2b42a714a
This commit is contained in:
David Pursehouse
2017-04-25 13:52:55 +02:00
parent 086836ac47 e838482e9e
commit cfac9d34d8
5 changed files with 83 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/AuthInfo.java
View File

@@ -84,7 +84,8 @@ public class AuthInfo extends JavaScriptObject {
}
public final boolean isHttpPasswordSettingsEnabled() {
return gitBasicAuthPolicy() != GitBasicAuthPolicy.LDAP;
return gitBasicAuthPolicy() == GitBasicAuthPolicy.HTTP
|| gitBasicAuthPolicy() == GitBasicAuthPolicy.HTTP_LDAP;
}
public final GitBasicAuthPolicy gitBasicAuthPolicy() {

4
gerrit-gwtui/src/main/java/com/google/gerrit/client/account/SettingsScreen.java
View File

@@ -21,6 +21,7 @@ import com.google.gerrit.client.api.ExtensionSettingsScreen;
import com.google.gerrit.client.rpc.Natives;
import com.google.gerrit.client.ui.MenuScreen;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.extensions.client.GitBasicAuthPolicy;
import java.util.HashSet;
import java.util.Set;
@@ -46,7 +47,8 @@ public abstract class SettingsScreen extends MenuScreen {
if (Gerrit.info().auth().isHttpPasswordSettingsEnabled()) {
linkByGerrit(Util.C.tabHttpAccess(), PageLinks.SETTINGS_HTTP_PASSWORD);
}
if (Gerrit.info().auth().isOAuth()) {
if (Gerrit.info().auth().isOAuth()
&& Gerrit.info().auth().gitBasicAuthPolicy() == GitBasicAuthPolicy.OAUTH) {
linkByGerrit(Util.C.tabOAuthToken(), PageLinks.SETTINGS_OAUTH_TOKEN);
}
if (Gerrit.info().gerrit().editGpgKeys()) {

25
gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java
View File

@@ -14,10 +14,13 @@
package com.google.gerrit.server.change;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gwtorm.server.OrmException;
import java.util.Collection;
import java.util.Optional;
/**
* Store for reviewed flags on changes.
@@ -30,6 +33,19 @@ import java.util.Collection;
* <p>For a multi-master setup the store must replicate the data between the masters.
*/
public interface AccountPatchReviewStore {
/** Represents patch set id with reviewed files. */
@AutoValue
abstract class PatchSetWithReviewedFiles {
abstract PatchSet.Id patchSetId();
abstract ImmutableSet<String> files();
public static PatchSetWithReviewedFiles create(PatchSet.Id id, ImmutableSet<String> files) {
return new AutoValue_AccountPatchReviewStore_PatchSetWithReviewedFiles(id, files);
}
}
/**
* Marks the given file in the given patch set as reviewed by the given user.
*
@@ -72,12 +88,15 @@ public interface AccountPatchReviewStore {
void clearReviewed(PatchSet.Id psId) throws OrmException;
/**
* Returns the paths of all files in the given patch set the have been reviewed by the given user.
* Find the latest patch set, that is smaller or equals to the given patch set, where at least,
* one file has been reviewed by the given user.
*
* @param psId patch set ID
* @param accountId account ID of the user
* @return the paths of all files in the given patch set the have been reviewed by the given user
* @return optionally, all files the have been reviewed by the given user that belong to the patch
* set that is smaller or equals to the given patch set
* @throws OrmException thrown if accessing the reviewed flags failed
*/
Collection<String> findReviewed(PatchSet.Id psId, Account.Id accountId) throws OrmException;
Optional<PatchSetWithReviewedFiles> findReviewed(PatchSet.Id psId, Account.Id accountId)
throws OrmException;
}

39
gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.server.change;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -34,6 +33,7 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.AccountPatchReviewStore.PatchSetWithReviewedFiles;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
@@ -45,8 +45,10 @@ import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -222,31 +224,24 @@ public class Files implements ChildCollection<RevisionResource, FileResource> {
}
Account.Id userId = user.getAccountId();
Collection<String> r =
accountPatchReviewStore.get().findReviewed(resource.getPatchSet().getId(), userId);
PatchSet patchSetId = resource.getPatchSet();
Optional<PatchSetWithReviewedFiles> o =
accountPatchReviewStore.get().findReviewed(patchSetId.getId(), userId);
if (r.isEmpty() && 1 < resource.getPatchSet().getPatchSetId()) {
for (PatchSet ps : reversePatchSets(resource)) {
Collection<String> o = accountPatchReviewStore.get().findReviewed(ps.getId(), userId);
if (!o.isEmpty()) {
try {
r = copy(Sets.newHashSet(o), ps.getId(), resource, userId);
} catch (IOException | PatchListNotAvailableException e) {
log.warn("Cannot copy patch review flags", e);
}
break;
}
if (o.isPresent()) {
PatchSetWithReviewedFiles res = o.get();
if (res.patchSetId().equals(patchSetId.getId())) {
return res.files();
}
try {
return copy(res.files(), res.patchSetId(), resource, userId);
} catch (IOException | PatchListNotAvailableException e) {
log.warn("Cannot copy patch review flags", e);
}
}
return r;
}
private List<PatchSet> reversePatchSets(RevisionResource resource) throws OrmException {
Collection<PatchSet> patchSets = psUtil.byChange(db.get(), resource.getNotes());
List<PatchSet> list =
(patchSets instanceof List) ? (List<PatchSet>) patchSets : new ArrayList<>(patchSets);
return Lists.reverse(list);
return Collections.emptyList();
}
private List<String> copy(

67
gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.schema;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicItem;
@@ -34,9 +35,8 @@ import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import javax.sql.DataSource;
import org.apache.commons.dbcp.BasicDataSource;
import org.eclipse.jgit.lib.Config;
@@ -130,20 +130,20 @@ public class H2AccountPatchReviewStore implements AccountPatchReviewStore, Lifec
private static void doCreateTable(Statement stmt) throws SQLException {
stmt.executeUpdate(
"CREATE TABLE IF NOT EXISTS ACCOUNT_PATCH_REVIEWS ("
+ "ACCOUNT_ID INTEGER DEFAULT 0 NOT NULL, "
+ "CHANGE_ID INTEGER DEFAULT 0 NOT NULL, "
+ "PATCH_SET_ID INTEGER DEFAULT 0 NOT NULL, "
+ "FILE_NAME VARCHAR(255) DEFAULT '' NOT NULL, "
+ "CONSTRAINT PRIMARY_KEY_ACCOUNT_PATCH_REVIEWS "
+ "PRIMARY KEY (ACCOUNT_ID, CHANGE_ID, PATCH_SET_ID, FILE_NAME)"
"CREATE TABLE IF NOT EXISTS account_patch_reviews ("
+ "account_id INTEGER DEFAULT 0 NOT NULL, "
+ "change_id INTEGER DEFAULT 0 NOT NULL, "
+ "patch_set_id INTEGER DEFAULT 0 NOT NULL, "
+ "file_name VARCHAR(4096) DEFAULT '' NOT NULL, "
+ "CONSTRAINT primary_key_account_patch_reviews "
+ "PRIMARY KEY (account_id, change_id, patch_set_id, file_name)"
+ ")");
}
public static void dropTableIfExists(String url) throws OrmException {
try (Connection con = DriverManager.getConnection(url);
Statement stmt = con.createStatement()) {
stmt.executeUpdate("DROP TABLE IF EXISTS ACCOUNT_PATCH_REVIEWS");
stmt.executeUpdate("DROP TABLE IF EXISTS account_patch_reviews");
} catch (SQLException e) {
throw convertError("create", e);
}
@@ -158,8 +158,8 @@ public class H2AccountPatchReviewStore implements AccountPatchReviewStore, Lifec
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
"INSERT INTO ACCOUNT_PATCH_REVIEWS "
+ "(ACCOUNT_ID, CHANGE_ID, PATCH_SET_ID, FILE_NAME) VALUES "
"INSERT INTO account_patch_reviews "
+ "(account_id, change_id, patch_set_id, file_name) VALUES "
+ "(?, ?, ?, ?)")) {
stmt.setInt(1, accountId.get());
stmt.setInt(2, psId.getParentKey().get());
@@ -186,8 +186,8 @@ public class H2AccountPatchReviewStore implements AccountPatchReviewStore, Lifec
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
"INSERT INTO ACCOUNT_PATCH_REVIEWS "
+ "(ACCOUNT_ID, CHANGE_ID, PATCH_SET_ID, FILE_NAME) VALUES "
"INSERT INTO account_patch_reviews "
+ "(account_id, change_id, patch_set_id, file_name) VALUES "
+ "(?, ?, ?, ?)")) {
for (String path : paths) {
stmt.setInt(1, accountId.get());
@@ -212,9 +212,9 @@ public class H2AccountPatchReviewStore implements AccountPatchReviewStore, Lifec
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
"DELETE FROM ACCOUNT_PATCH_REVIEWS "
+ "WHERE ACCOUNT_ID = ? AND CHANGE_ID + ? AND "
+ "PATCH_SET_ID = ? AND FILE_NAME = ?")) {
"DELETE FROM account_patch_reviews "
+ "WHERE account_id = ? AND change_id = ? AND "
+ "patch_set_id = ? AND file_name = ?")) {
stmt.setInt(1, accountId.get());
stmt.setInt(2, psId.getParentKey().get());
stmt.setInt(3, psId.get());
@@ -230,8 +230,8 @@ public class H2AccountPatchReviewStore implements AccountPatchReviewStore, Lifec
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
"DELETE FROM ACCOUNT_PATCH_REVIEWS "
+ "WHERE CHANGE_ID + ? AND PATCH_SET_ID = ?")) {
"DELETE FROM account_patch_reviews "
+ "WHERE change_id = ? AND patch_set_id = ?")) {
stmt.setInt(1, psId.getParentKey().get());
stmt.setInt(2, psId.get());
stmt.executeUpdate();
@@ -241,22 +241,33 @@ public class H2AccountPatchReviewStore implements AccountPatchReviewStore, Lifec
}
@Override
public Collection<String> findReviewed(PatchSet.Id psId, Account.Id accountId)
public Optional<PatchSetWithReviewedFiles> findReviewed(PatchSet.Id psId, Account.Id accountId)
throws OrmException {
try (Connection con = ds.getConnection();
PreparedStatement stmt =
con.prepareStatement(
"SELECT FILE_NAME FROM ACCOUNT_PATCH_REVIEWS "
+ "WHERE ACCOUNT_ID = ? AND CHANGE_ID = ? AND PATCH_SET_ID = ?")) {
"SELECT patch_set_id, file_name FROM account_patch_reviews APR1 "
+ "WHERE account_id = ? AND change_id = ? AND patch_set_id = "
+ "(SELECT MAX(patch_set_id) FROM account_patch_reviews APR2 WHERE "
+ "APR1.account_id = APR2.account_id "
+ "AND APR1.change_id = APR2.change_id "
+ "AND patch_set_id <= ?)")) {
stmt.setInt(1, accountId.get());
stmt.setInt(2, psId.getParentKey().get());
stmt.setInt(3, psId.get());
try (ResultSet rs = stmt.executeQuery()) {
List<String> files = new ArrayList<>();
while (rs.next()) {
files.add(rs.getString("FILE_NAME"));
if (rs.next()) {
PatchSet.Id id = new PatchSet.Id(psId.getParentKey(), rs.getInt("PATCH_SET_ID"));
ImmutableSet.Builder<String> builder = ImmutableSet.builder();
do {
builder.add(rs.getString("FILE_NAME"));
} while (rs.next());
return Optional.of(
AccountPatchReviewStore.PatchSetWithReviewedFiles.create(id, builder.build()));
}
return files;
return Optional.empty();
}
} catch (SQLException e) {
throw convertError("select", e);
@@ -267,13 +278,13 @@ public class H2AccountPatchReviewStore implements AccountPatchReviewStore, Lifec
switch (getSQLStateInt(err)) {
case 23001: // UNIQUE CONSTRAINT VIOLATION
case 23505: // DUPLICATE_KEY_1
return new OrmDuplicateKeyException("ACCOUNT_PATCH_REVIEWS", err);
return new OrmDuplicateKeyException("account_patch_reviews", err);
default:
if (err.getCause() == null && err.getNextException() != null) {
err.initCause(err.getNextException());
}
return new OrmException(op + " failure on ACCOUNT_PATCH_REVIEWS", err);
return new OrmException(op + " failure on account_patch_reviews", err);
}
}