From 522ffdfaad8cc5400ec03a5b43d914b7f67b69d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 20 Dec 2019 13:39:08 +0100 Subject: [PATCH 1/4] Don't check conflicting ref names when deleting refs The Schema_167 migration was very slow on our production data with about 30K user refs and about 20K groups. This was caused by scanning for all refs when deleting a ref during migration When deleting a ref we do not need to check for the conflicting refs name and can thus avoid the expensive scan for all refs. Change-Id: I1168bd192fa9c3c8c0791641ef063955d03f35ec --- java/com/google/gerrit/server/update/RefUpdateUtil.java | 1 + 1 file changed, 1 insertion(+) diff --git a/java/com/google/gerrit/server/update/RefUpdateUtil.java b/java/com/google/gerrit/server/update/RefUpdateUtil.java index 3e336776bf..71eef9e38b 100644 --- a/java/com/google/gerrit/server/update/RefUpdateUtil.java +++ b/java/com/google/gerrit/server/update/RefUpdateUtil.java @@ -119,6 +119,7 @@ public class RefUpdateUtil { public static void deleteChecked(Repository repo, String refName) throws IOException { RefUpdate ru = repo.updateRef(refName); ru.setForceUpdate(true); + ru.setCheckConflicting(false); switch (ru.delete()) { case FORCED: // Ref was deleted. From 57993866d8634da9398a87f77ab7737aa0f56300 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 10 Oct 2019 17:55:34 +0200 Subject: [PATCH 2/4] Fix a bug where _moreChanges is set to the wrong value The query changes REST endpoint supports sending multiple queries in a single request. The backend implementation uses a cache (a simple map) to spare computing a ChangeInfo object more than once in case it shows up in more than a single query result. The implementation contained a bug that would set _moreChanges on the last entity of one query that has more results than get sent. However, through the reuse of the entity, this boolean would also end up in other entities, where it's simply wrong. There are multiple, alternative ways how this can be fixed: 1) Remove the cache: We could simply remove the map. This has an unknown performance impact. 2) Deep-copy objects instead of plainly reusing them: This would add boiler-plate to a lot of *Info objects that ChangeInfo depends on. Hopefully we can consider moving to immutable API objects (AutoValue or Protobuf) in the future, which would obsolete this code. It still seems undesirable to add this now. Notably, ActionJson already contains what looks like brittle copy logic. 3) Rework the JSON response to be a List where QueryResponse is an object that has List and _more: Would be the right path design-wise, but is a non-starter because it's a major change to a heavily used API. 4) Never reuse the last entity in a result set: In this way, the last entity is always unique. Setting _moreChanges on it has no side effects. This commit implements (4), which seems like the least evil. Change-Id: I6b007d442249d0445ea49f125c45d8f9022e104e --- .../gerrit/server/change/ChangeJson.java | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index 05cd2358ce..2aaf4c513c 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -296,7 +296,6 @@ public class ChangeJson { Map cache = Maps.newHashMapWithExpectedSize(in.size()); for (QueryResult r : in) { List infos = toChangeInfos(r.entities(), cache); - infos.forEach(c -> cache.put(new Change.Id(c._number), c)); if (!infos.isEmpty() && r.more()) { infos.get(infos.size() - 1)._moreChanges = true; } @@ -417,14 +416,29 @@ public class ChangeJson { List changes, Map cache) { try (Timer0.Context ignored = metrics.toChangeInfosLatency.start()) { List changeInfos = new ArrayList<>(changes.size()); - for (ChangeData cd : changes) { - ChangeInfo i = cache.get(cd.getId()); - if (i != null) { + for (int i = 0; i < changes.size(); i++) { + // We can only cache and re-use an entity if it's not the last in the list. The last entity + // may later get _moreChanges set. If it was cached or re-used, that setting would propagate + // to the original entity yielding wrong results. + // This problem has two sides where 'last in the list' has to be respected: + // (1) Caching + // (2) Reusing + boolean isCacheable = i != changes.size() - 1; + ChangeData cd = changes.get(i); + ChangeInfo info = cache.get(cd.getId()); + if (info != null && isCacheable) { + changeInfos.add(info); continue; } + + // Compute and cache if possible try { ensureLoaded(Collections.singleton(cd)); - changeInfos.add(format(cd, Optional.empty(), false, ChangeInfo::new)); + info = format(cd, Optional.empty(), false, ChangeInfo::new); + changeInfos.add(info); + if (isCacheable) { + cache.put(new Change.Id(info._number), info); + } } catch (OrmException | RuntimeException e) { logger.atWarning().withCause(e).log( "Omitting corrupt change %s from results", cd.getId()); From bc6f62c5998bc748341ac28e400a666c5c471854 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 15 Jan 2020 20:32:09 +0100 Subject: [PATCH 3/4] Remove orphan library downloader facility The last usage was removed in: I6963ee23af. Change-Id: I3e1e71691c1aae4e97768993dc25e04c2d674181 --- .../google/gerrit/pgm/init/InitModule.java | 2 - .../com/google/gerrit/pgm/init/Libraries.java | 141 -------- .../gerrit/pgm/init/LibraryDownloader.java | 316 ------------------ .../google/gerrit/pgm/init/LibrariesTest.java | 46 --- .../google/gerrit/pgm/init/libraries.config | 50 --- 5 files changed, 555 deletions(-) delete mode 100644 java/com/google/gerrit/pgm/init/Libraries.java delete mode 100644 java/com/google/gerrit/pgm/init/LibraryDownloader.java delete mode 100644 javatests/com/google/gerrit/pgm/init/LibrariesTest.java delete mode 100644 resources/com/google/gerrit/pgm/init/libraries.config diff --git a/java/com/google/gerrit/pgm/init/InitModule.java b/java/com/google/gerrit/pgm/init/InitModule.java index f2fc001d04..b65867538b 100644 --- a/java/com/google/gerrit/pgm/init/InitModule.java +++ b/java/com/google/gerrit/pgm/init/InitModule.java @@ -34,8 +34,6 @@ public class InitModule extends FactoryModule { @Override protected void configure() { bind(SitePaths.class); - bind(Libraries.class); - bind(LibraryDownloader.class); factory(Section.Factory.class); factory(VersionedAuthorizedKeysOnInit.Factory.class); diff --git a/java/com/google/gerrit/pgm/init/Libraries.java b/java/com/google/gerrit/pgm/init/Libraries.java deleted file mode 100644 index c599e99eb8..0000000000 --- a/java/com/google/gerrit/pgm/init/Libraries.java +++ /dev/null @@ -1,141 +0,0 @@ -// Copyright (C) 2009 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.pgm.init; - -import static java.nio.charset.StandardCharsets.UTF_8; - -import com.google.gerrit.pgm.init.api.LibraryDownload; -import com.google.inject.Inject; -import com.google.inject.Provider; -import com.google.inject.Singleton; -import java.io.FileNotFoundException; -import java.io.IOException; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.Reader; -import java.lang.reflect.Field; -import java.lang.reflect.Modifier; -import java.util.List; -import org.eclipse.jgit.errors.ConfigInvalidException; -import org.eclipse.jgit.lib.Config; - -/** Standard {@link LibraryDownloader} instances derived from configuration. */ -@Singleton -class Libraries { - private static final String RESOURCE_FILE = "com/google/gerrit/pgm/init/libraries.config"; - - private final Provider downloadProvider; - private final List skippedDownloads; - private final boolean skipAllDownloads; - - /* final */ LibraryDownloader db2Driver; - /* final */ LibraryDownloader db2DriverLicense; - /* final */ LibraryDownloader hanaDriver; - /* final */ LibraryDownloader mariadbDriver; - /* final */ LibraryDownloader mysqlDriver; - /* final */ LibraryDownloader oracleDriver; - - @Inject - Libraries( - final Provider downloadProvider, - @LibraryDownload List skippedDownloads, - @LibraryDownload Boolean skipAllDownloads) { - this.downloadProvider = downloadProvider; - this.skippedDownloads = skippedDownloads; - this.skipAllDownloads = skipAllDownloads; - init(); - } - - private void init() { - final Config cfg = new Config(); - try { - cfg.fromText(read(RESOURCE_FILE)); - } catch (IOException | ConfigInvalidException e) { - throw new RuntimeException(e.getMessage(), e); - } - - for (Field f : Libraries.class.getDeclaredFields()) { - if ((f.getModifiers() & Modifier.STATIC) == 0 && f.getType() == LibraryDownloader.class) { - try { - f.set(this, downloadProvider.get()); - } catch (IllegalArgumentException | IllegalAccessException e) { - throw new IllegalStateException("Cannot initialize " + f.getName()); - } - } - } - - for (Field f : Libraries.class.getDeclaredFields()) { - if ((f.getModifiers() & Modifier.STATIC) == 0 && f.getType() == LibraryDownloader.class) { - try { - init(f, cfg); - } catch (IllegalArgumentException - | IllegalAccessException - | NoSuchFieldException - | SecurityException e) { - throw new IllegalStateException("Cannot configure " + f.getName()); - } - } - } - } - - private void init(Field field, Config cfg) - throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, - SecurityException { - String n = field.getName(); - LibraryDownloader dl = (LibraryDownloader) field.get(this); - dl.setName(get(cfg, n, "name")); - dl.setJarUrl(get(cfg, n, "url")); - dl.setSHA1(getOptional(cfg, n, "sha1")); - dl.setRemove(get(cfg, n, "remove")); - for (String d : cfg.getStringList("library", n, "needs")) { - dl.addNeeds((LibraryDownloader) getClass().getDeclaredField(d).get(this)); - } - dl.setSkipDownload(skipAllDownloads || skippedDownloads.contains(n)); - } - - private static String getOptional(Config cfg, String name, String key) { - return doGet(cfg, name, key, false); - } - - private static String get(Config cfg, String name, String key) { - return doGet(cfg, name, key, true); - } - - private static String doGet(Config cfg, String name, String key, boolean required) { - String val = cfg.getString("library", name, key); - if ((val == null || val.isEmpty()) && required) { - throw new IllegalStateException( - "Variable library." + name + "." + key + " is required within " + RESOURCE_FILE); - } - return val; - } - - private static String read(String p) throws IOException { - try (InputStream in = Libraries.class.getClassLoader().getResourceAsStream(p)) { - if (in == null) { - throw new FileNotFoundException("Cannot load resource " + p); - } - try (Reader r = new InputStreamReader(in, UTF_8)) { - final StringBuilder buf = new StringBuilder(); - final char[] tmp = new char[512]; - int n; - while (0 < (n = r.read(tmp))) { - buf.append(tmp, 0, n); - } - return buf.toString(); - } - } - } -} diff --git a/java/com/google/gerrit/pgm/init/LibraryDownloader.java b/java/com/google/gerrit/pgm/init/LibraryDownloader.java deleted file mode 100644 index 0b31ee22ab..0000000000 --- a/java/com/google/gerrit/pgm/init/LibraryDownloader.java +++ /dev/null @@ -1,316 +0,0 @@ -// Copyright (C) 2009 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.pgm.init; - -import com.google.common.hash.Funnels; -import com.google.common.hash.Hasher; -import com.google.common.hash.Hashing; -import com.google.common.io.ByteStreams; -import com.google.gerrit.common.Die; -import com.google.gerrit.common.IoUtil; -import com.google.gerrit.pgm.init.api.ConsoleUI; -import com.google.gerrit.server.config.SitePaths; -import com.google.inject.Inject; -import java.io.FileNotFoundException; -import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; -import java.net.HttpURLConnection; -import java.net.Proxy; -import java.net.ProxySelector; -import java.net.URISyntaxException; -import java.net.URL; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.ArrayList; -import java.util.List; -import org.eclipse.jgit.util.HttpSupport; - -/** Get optional or required 3rd party library files into $site_path/lib. */ -class LibraryDownloader { - private final ConsoleUI ui; - private final Path lib_dir; - private final StaleLibraryRemover remover; - - private boolean required; - private String name; - private String jarUrl; - private String sha1; - private String remove; - private List needs; - private LibraryDownloader neededBy; - private Path dst; - private boolean download; // download or copy - private boolean exists; - private boolean skipDownload; - - @Inject - LibraryDownloader(ConsoleUI ui, SitePaths site, StaleLibraryRemover remover) { - this.ui = ui; - this.lib_dir = site.lib_dir; - this.remover = remover; - this.needs = new ArrayList<>(2); - } - - void setName(String name) { - this.name = name; - } - - void setJarUrl(String url) { - this.jarUrl = url; - download = jarUrl.startsWith("http"); - } - - void setSHA1(String sha1) { - this.sha1 = sha1; - } - - void setRemove(String remove) { - this.remove = remove; - } - - void addNeeds(LibraryDownloader lib) { - needs.add(lib); - } - - void setSkipDownload(boolean skipDownload) { - this.skipDownload = skipDownload; - } - - void downloadRequired() { - setRequired(true); - download(); - } - - void downloadOptional() { - required = false; - download(); - } - - private void setRequired(boolean r) { - required = r; - for (LibraryDownloader d : needs) { - d.setRequired(r); - } - } - - private void download() { - if (skipDownload) { - return; - } - - if (jarUrl == null || !jarUrl.contains("/")) { - throw new IllegalStateException("Invalid JarUrl for " + name); - } - - final String jarName = jarUrl.substring(jarUrl.lastIndexOf('/') + 1); - if (jarName.contains("/") || jarName.contains("\\")) { - throw new IllegalStateException("Invalid JarUrl: " + jarUrl); - } - - if (name == null) { - name = jarName; - } - - dst = lib_dir.resolve(jarName); - if (Files.exists(dst)) { - exists = true; - } else if (shouldGet()) { - doGet(); - } - - if (exists) { - for (LibraryDownloader d : needs) { - d.neededBy = this; - d.downloadRequired(); - } - } - } - - private boolean shouldGet() { - if (ui.isBatch()) { - return required; - } - final StringBuilder msg = new StringBuilder(); - msg.append("\n"); - msg.append("Gerrit Code Review is not shipped with %s\n"); - if (neededBy != null) { - msg.append(String.format("** This library is required by %s. **\n", neededBy.name)); - } else if (required) { - msg.append("** This library is required for your configuration. **\n"); - } else { - msg.append(" If available, Gerrit can take advantage of features\n"); - msg.append(" in the library, but will also function without it.\n"); - } - msg.append(String.format("%s and install it now", download ? "Download" : "Copy")); - return ui.yesno(true, msg.toString(), name); - } - - private void doGet() { - if (!Files.exists(lib_dir)) { - try { - Files.createDirectories(lib_dir); - } catch (IOException e) { - throw new Die("Cannot create " + lib_dir, e); - } - } - - try { - remover.remove(remove); - if (download) { - doGetByHttp(); - } else { - doGetByLocalCopy(); - } - verifyFileChecksum(); - } catch (IOException err) { - try { - Files.delete(dst); - } catch (IOException e) { - // Delete failed; leave alone. - } - - if (ui.isBatch()) { - throw new Die("error: Cannot get " + jarUrl, err); - } - - System.err.println(); - System.err.println(); - System.err.println("error: " + err.getMessage()); - System.err.println("Please download:"); - System.err.println(); - System.err.println(" " + jarUrl); - System.err.println(); - System.err.println("and save as:"); - System.err.println(); - System.err.println(" " + dst.toAbsolutePath()); - System.err.println(); - System.err.flush(); - - ui.waitForUser(); - - if (Files.exists(dst)) { - verifyFileChecksum(); - - } else if (!ui.yesno(!required, "Continue without this library")) { - throw new Die("aborted by user"); - } - } - - if (Files.exists(dst)) { - exists = true; - IoUtil.loadJARs(dst); - } - } - - private void doGetByLocalCopy() throws IOException { - System.err.print("Copying " + jarUrl + " ..."); - Path p = url2file(jarUrl); - if (!Files.exists(p)) { - StringBuilder msg = - new StringBuilder() - .append("\n") - .append("Can not find the %s at this location: %s\n") - .append("Please provide alternative URL"); - p = url2file(ui.readString(null, msg.toString(), name, jarUrl)); - } - Files.copy(p, dst); - } - - private static Path url2file(String urlString) throws IOException { - final URL url = new URL(urlString); - try { - return Paths.get(url.toURI()); - } catch (URISyntaxException e) { - return Paths.get(url.getPath()); - } - } - - private void doGetByHttp() throws IOException { - System.err.print("Downloading " + jarUrl + " ..."); - System.err.flush(); - try (InputStream in = openHttpStream(jarUrl); - OutputStream out = Files.newOutputStream(dst)) { - ByteStreams.copy(in, out); - System.err.println(" OK"); - System.err.flush(); - } catch (IOException err) { - deleteDst(); - System.err.println(" !! FAIL !!"); - System.err.println(err); - System.err.flush(); - throw err; - } - } - - private static InputStream openHttpStream(String urlStr) throws IOException { - ProxySelector proxySelector = ProxySelector.getDefault(); - URL url = new URL(urlStr); - Proxy proxy = HttpSupport.proxyFor(proxySelector, url); - HttpURLConnection c = (HttpURLConnection) url.openConnection(proxy); - - switch (HttpSupport.response(c)) { - case HttpURLConnection.HTTP_OK: - return c.getInputStream(); - - case HttpURLConnection.HTTP_NOT_FOUND: - throw new FileNotFoundException(url.toString()); - - default: - throw new IOException( - url.toString() + ": " + HttpSupport.response(c) + " " + c.getResponseMessage()); - } - } - - @SuppressWarnings("deprecation") // Use Hashing.sha1 for compatibility. - private void verifyFileChecksum() { - if (sha1 == null) { - System.err.println(); - System.err.flush(); - return; - } - Hasher h = Hashing.sha1().newHasher(); - try (InputStream in = Files.newInputStream(dst); - OutputStream out = Funnels.asOutputStream(h)) { - ByteStreams.copy(in, out); - } catch (IOException e) { - deleteDst(); - throw new Die("cannot checksum " + dst, e); - } - if (sha1.equals(h.hash().toString())) { - System.err.println("Checksum " + dst.getFileName() + " OK"); - System.err.flush(); - } else if (ui.isBatch()) { - deleteDst(); - throw new Die(dst + " SHA-1 checksum does not match"); - - } else if (!ui.yesno( - null /* force an answer */, - "error: SHA-1 checksum does not match\nUse %s anyway", // - dst.getFileName())) { - deleteDst(); - throw new Die("aborted by user"); - } - } - - private void deleteDst() { - try { - Files.delete(dst); - } catch (IOException e) { - System.err.println(" Failed to clean up lib: " + dst); - } - } -} diff --git a/javatests/com/google/gerrit/pgm/init/LibrariesTest.java b/javatests/com/google/gerrit/pgm/init/LibrariesTest.java deleted file mode 100644 index 47b75092ec..0000000000 --- a/javatests/com/google/gerrit/pgm/init/LibrariesTest.java +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright (C) 2009 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.pgm.init; - -import static org.easymock.EasyMock.createStrictMock; -import static org.easymock.EasyMock.replay; -import static org.easymock.EasyMock.verify; -import static org.junit.Assert.assertNotNull; - -import com.google.gerrit.pgm.init.api.ConsoleUI; -import com.google.gerrit.server.config.SitePaths; -import com.google.gerrit.testing.GerritBaseTests; -import java.nio.file.Paths; -import java.util.Collections; -import org.junit.Test; - -public class LibrariesTest extends GerritBaseTests { - @Test - public void create() throws Exception { - final SitePaths site = new SitePaths(Paths.get(".")); - final ConsoleUI ui = createStrictMock(ConsoleUI.class); - final StaleLibraryRemover remover = createStrictMock(StaleLibraryRemover.class); - - replay(ui); - - Libraries lib = - new Libraries( - () -> new LibraryDownloader(ui, site, remover), Collections.emptyList(), false); - - assertNotNull(lib.mysqlDriver); - - verify(ui); - } -} diff --git a/resources/com/google/gerrit/pgm/init/libraries.config b/resources/com/google/gerrit/pgm/init/libraries.config deleted file mode 100644 index 3d3545b150..0000000000 --- a/resources/com/google/gerrit/pgm/init/libraries.config +++ /dev/null @@ -1,50 +0,0 @@ -# Copyright (C) 2009 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. - -[library "mysqlDriver"] - name = MySQL Connector/J 5.1.43 - url = https://repo1.maven.org/maven2/mysql/mysql-connector-java/5.1.43/mysql-connector-java-5.1.43.jar - sha1 = dee9103eec0d877f3a21c82d4d9e9f4fbd2d6e0a - remove = mysql-connector-java-.*[.]jar - -[library "mariadbDriver"] - name = MariaDB Connector/J 2.3.0 - url = https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client/2.3.0/mariadb-java-client-2.3.0.jar - sha1 = c2b1a6002a169757d0649449288e9b3b776af76b - remove = mariadb-java-client-.*[.]jar - -[library "oracleDriver"] - name = Oracle JDBC driver 11g Release 2 (11.2.0) - url = file:///u01/app/oracle/product/11.2.0/xe/jdbc/lib/ojdbc6.jar - sha1 = 2f89cd9176772c3a6c261ce6a8e3d0d4425f5679 - remove = ojdbc6.jar - -[library "db2Driver"] - name = DB2 Type 4 JDBC driver (10.5) - url = file:///opt/ibm/db2/V10.5/java/db2jcc4.jar - sha1 = 9344d4fd41d6511f2d1d1deb7759056495b3a39b - needs = db2DriverLicense - remove = db2jcc4.jar - -# Omit SHA-1 for license JAR as it's not stable and depends on the product -# the customer has purchased. -[library "db2DriverLicense"] - name = DB2 Type 4 JDBC driver license (10.5) - url = file:///opt/ibm/db2/V10.5/java/db2jcc_license_cu.jar - remove = db2jcc_license_cu.jar - -[library "hanaDriver"] - name = HANA JDBC driver - url = file:///usr/sap/hdbclient/ngdbc.jar - remove = ngdbc.jar From 36dcc21aad636efa1144c120e06c913e41f0f493 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 15 Jan 2020 13:58:54 +0100 Subject: [PATCH 4/4] ChangeEditModifier: Reject invalid file paths as '400 Bad Request' Providing an invalid path is a user error and should not result in a '500 Internal Server Error' [1]. [1] AutoRetry: restapi.change.ChangeEdits.Post failed, retry with tracing enabled org.eclipse.jgit.dircache.InvalidPathException: Invalid path: foo/bar/ at org.eclipse.jgit.dircache.DirCacheEntry.checkPath(DirCacheEntry.java:837) at org.eclipse.jgit.dircache.DirCacheEntry.(DirCacheEntry.java:284) at org.eclipse.jgit.dircache.DirCacheEntry.(DirCacheEntry.java:266) at org.eclipse.jgit.dircache.DirCacheEditor.applyEdits(DirCacheEditor.java:163) at org.eclipse.jgit.dircache.DirCacheEditor.finish(DirCacheEditor.java:132) at com.google.gerrit.server.edit.tree.TreeCreator.applyPathEdits(TreeCreator.java:99) at com.google.gerrit.server.edit.tree.TreeCreator.createNewTree(TreeCreator.java:73) at com.google.gerrit.server.edit.tree.TreeCreator.createNewTreeAndGetId(TreeCreator.java:66) at com.google.gerrit.server.edit.ChangeEditModifier.createNewTree(ChangeEditModifier.java:492) at com.google.gerrit.server.edit.ChangeEditModifier.modifyTree(ChangeEditModifier.java:333) at com.google.gerrit.server.edit.ChangeEditModifier.renameFile(ChangeEditModifier.java:300) at com.google.gerrit.server.restapi.change.ChangeEdits$Post.apply(ChangeEdits.java:248) at com.google.gerrit.server.restapi.change.ChangeEdits$Post.apply(ChangeEdits.java:222) at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestCollectionModifyViewWithRetry$10(RestApiServlet.java:858) at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78) at com.github.rholder.retry.Retryer.call(Retryer.java:160) at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:560) at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:503) at com.google.gerrit.server.update.RetryableAction.call(RetryableAction.java:172) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:883) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestCollectionModifyViewWithRetry(RestApiServlet.java:853) at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:563) at javax.servlet.http.HttpServlet.service(HttpServlet.java:717) ... Signed-off-by: Edwin Kempin Change-Id: I7693b4a6c39d7b773101ab5770b158433995f23d --- .../server/edit/ChangeEditModifier.java | 31 ++++++++++++------- .../server/restapi/change/ApplyFix.java | 5 +-- .../server/restapi/change/ChangeEdits.java | 21 ++++++++----- .../gerrit/acceptance/edit/ChangeEditIT.java | 12 +++++++ 4 files changed, 49 insertions(+), 20 deletions(-) diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 6295e2d3b1..246a53f124 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -51,6 +51,7 @@ import java.sql.Timestamp; import java.util.List; import java.util.Optional; import java.util.TimeZone; +import org.eclipse.jgit.dircache.InvalidPathException; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.NullProgressMonitor; @@ -243,13 +244,14 @@ public class ChangeEditModifier { * @param filePath the path of the file whose contents should be modified * @param newContent the new file content * @throws AuthException if the user isn't authenticated or not allowed to use change edits + * @throws BadRequestException if the user provided bad input (e.g. invalid file paths) * @throws InvalidChangeOperationException if the file already had the specified content * @throws PermissionBackendException * @throws ResourceConflictException if the project state does not permit the operation */ public void modifyFile( Repository repository, ChangeNotes notes, String filePath, RawInput newContent) - throws AuthException, InvalidChangeOperationException, IOException, + throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { modifyTree(repository, notes, new ChangeFileContentModification(filePath, newContent)); } @@ -262,12 +264,13 @@ public class ChangeEditModifier { * @param notes the {@link ChangeNotes} of the change whose change edit should be modified * @param file path of the file which should be deleted * @throws AuthException if the user isn't authenticated or not allowed to use change edits + * @throws BadRequestException if the user provided bad input (e.g. invalid file paths) * @throws InvalidChangeOperationException if the file does not exist * @throws PermissionBackendException * @throws ResourceConflictException if the project state does not permit the operation */ public void deleteFile(Repository repository, ChangeNotes notes, String file) - throws AuthException, InvalidChangeOperationException, IOException, + throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { modifyTree(repository, notes, new DeleteFileModification(file)); } @@ -281,6 +284,7 @@ public class ChangeEditModifier { * @param currentFilePath the current path/name of the file * @param newFilePath the desired path/name of the file * @throws AuthException if the user isn't authenticated or not allowed to use change edits + * @throws BadRequestException if the user provided bad input (e.g. invalid file paths) * @throws InvalidChangeOperationException if the file was already renamed to the specified new * name * @throws PermissionBackendException @@ -288,7 +292,7 @@ public class ChangeEditModifier { */ public void renameFile( Repository repository, ChangeNotes notes, String currentFilePath, String newFilePath) - throws AuthException, InvalidChangeOperationException, IOException, + throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { modifyTree(repository, notes, new RenameFileModification(currentFilePath, newFilePath)); } @@ -306,14 +310,14 @@ public class ChangeEditModifier { * @throws PermissionBackendException */ public void restoreFile(Repository repository, ChangeNotes notes, String file) - throws AuthException, InvalidChangeOperationException, IOException, + throws AuthException, BadRequestException, InvalidChangeOperationException, IOException, PermissionBackendException, ResourceConflictException { modifyTree(repository, notes, new RestoreFileModification(file)); } private void modifyTree( Repository repository, ChangeNotes notes, TreeModification treeModification) - throws AuthException, IOException, InvalidChangeOperationException, + throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, PermissionBackendException, ResourceConflictException { assertCanEdit(notes); @@ -358,8 +362,8 @@ public class ChangeEditModifier { ChangeNotes notes, PatchSet patchSet, List treeModifications) - throws AuthException, IOException, InvalidChangeOperationException, MergeConflictException, - PermissionBackendException, ResourceConflictException { + throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, + MergeConflictException, PermissionBackendException, ResourceConflictException { assertCanEdit(notes); Optional optionalChangeEdit = lookupChangeEdit(notes); @@ -469,10 +473,15 @@ public class ChangeEditModifier { private static ObjectId createNewTree( Repository repository, RevCommit baseCommit, List treeModifications) - throws IOException, InvalidChangeOperationException { - TreeCreator treeCreator = new TreeCreator(baseCommit); - treeCreator.addTreeModifications(treeModifications); - ObjectId newTreeId = treeCreator.createNewTreeAndGetId(repository); + throws BadRequestException, IOException, InvalidChangeOperationException { + ObjectId newTreeId; + try { + TreeCreator treeCreator = new TreeCreator(baseCommit); + treeCreator.addTreeModifications(treeModifications); + newTreeId = treeCreator.createNewTreeAndGetId(repository); + } catch (InvalidPathException e) { + throw new BadRequestException(e.getMessage()); + } if (ObjectId.equals(newTreeId, baseCommit.getTree())) { throw new InvalidChangeOperationException("no changes were made"); diff --git a/java/com/google/gerrit/server/restapi/change/ApplyFix.java b/java/com/google/gerrit/server/restapi/change/ApplyFix.java index 231d3566d0..085f9dee83 100644 --- a/java/com/google/gerrit/server/restapi/change/ApplyFix.java +++ b/java/com/google/gerrit/server/restapi/change/ApplyFix.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.change; import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; @@ -66,8 +67,8 @@ public class ApplyFix implements RestModifyView { @Override public Response apply(FixResource fixResource, Void nothing) - throws AuthException, ResourceConflictException, IOException, ResourceNotFoundException, - PermissionBackendException { + throws AuthException, BadRequestException, ResourceConflictException, IOException, + ResourceNotFoundException, PermissionBackendException { RevisionResource revisionResource = fixResource.getRevisionResource(); Project.NameKey project = revisionResource.getProject(); ProjectState projectState = projectCache.checkedGet(project); diff --git a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java index 424f4d7f2c..d4cc03445a 100644 --- a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java +++ b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java @@ -118,7 +118,8 @@ public class ChangeEdits implements ChildCollection apply(ChangeResource resource, IdString id, Put.Input input) - throws AuthException, ResourceConflictException, IOException, PermissionBackendException { + throws AuthException, ResourceConflictException, BadRequestException, IOException, + PermissionBackendException { putEdit.apply(resource, id.get(), input.content); return Response.none(); } @@ -135,7 +136,8 @@ public class ChangeEdits implements ChildCollection apply(ChangeResource rsrc, IdString id, Input in) - throws IOException, AuthException, ResourceConflictException, PermissionBackendException { + throws IOException, AuthException, BadRequestException, ResourceConflictException, + PermissionBackendException { return deleteContent.apply(rsrc, id.get()); } } @@ -236,7 +238,8 @@ public class ChangeEdits implements ChildCollection apply(ChangeResource resource, Post.Input input) - throws AuthException, IOException, ResourceConflictException, PermissionBackendException { + throws AuthException, BadRequestException, IOException, ResourceConflictException, + PermissionBackendException { Project.NameKey project = resource.getProject(); try (Repository repository = repositoryManager.openRepository(project)) { if (isRestoreFile(input)) { @@ -281,12 +284,14 @@ public class ChangeEdits implements ChildCollection apply(ChangeEditResource rsrc, Input input) - throws AuthException, ResourceConflictException, IOException, PermissionBackendException { + throws AuthException, ResourceConflictException, BadRequestException, IOException, + PermissionBackendException { return apply(rsrc.getChangeResource(), rsrc.getPath(), input.content); } public Response apply(ChangeResource rsrc, String path, RawInput newContent) - throws ResourceConflictException, AuthException, IOException, PermissionBackendException { + throws ResourceConflictException, AuthException, BadRequestException, IOException, + PermissionBackendException { if (Strings.isNullOrEmpty(path) || path.charAt(0) == '/') { throw new ResourceConflictException("Invalid path: " + path); } @@ -320,12 +325,14 @@ public class ChangeEdits implements ChildCollection apply(ChangeEditResource rsrc, Input input) - throws AuthException, ResourceConflictException, IOException, PermissionBackendException { + throws AuthException, BadRequestException, ResourceConflictException, IOException, + PermissionBackendException { return apply(rsrc.getChangeResource(), rsrc.getPath()); } public Response apply(ChangeResource rsrc, String filePath) - throws AuthException, IOException, ResourceConflictException, PermissionBackendException { + throws AuthException, BadRequestException, IOException, ResourceConflictException, + PermissionBackendException { try (Repository repository = repositoryManager.openRepository(rsrc.getProject())) { editModifier.deleteFile(repository, rsrc.getNotes(), filePath); } catch (InvalidChangeOperationException e) { diff --git a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java index 6827219734..1317eadfb7 100644 --- a/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java +++ b/javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java @@ -24,6 +24,7 @@ import static com.google.gerrit.extensions.common.testing.EditInfoSubject.assert import static com.google.gerrit.extensions.restapi.testing.BinaryResultSubject.assertThat; import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.stream.Collectors.toList; @@ -52,6 +53,7 @@ import com.google.gerrit.extensions.common.DiffInfo; import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.FileInfo; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.PatchSet; @@ -435,6 +437,16 @@ public class ChangeEditIT extends AbstractDaemonTest { assertThat(getFileContentOfEdit(changeId, FILE_NAME)).isAbsent(); } + @Test + public void renameExistingFileToInvalidPath() throws Exception { + createEmptyEditFor(changeId); + BadRequestException badRequest = + assertThrows( + BadRequestException.class, + () -> gApi.changes().id(changeId).edit().renameFile(FILE_NAME, "invalid/path/")); + assertThat(badRequest.getMessage()).isEqualTo("Invalid path: invalid/path/"); + } + @Test public void createEditByDeletingExistingFileRest() throws Exception { adminRestSession.delete(urlEditFile(changeId, FILE_NAME)).assertNoContent();