opendev/gerrit
Code Issues Proposed changes

Store preferred download scheme as arbitrary strings

Browse Source 
Although plugins can register download schemes with arbitrary names,
the getter/setter in AccountGeneralPreferences were allowing only a
specific enum type corresponding to the core download schemes. Get rid
of that enum type, and store arbitrary strings in the database. To
avoid a schema upgrade (which would be an annoying multi-step process
for a zero-downtime upgrade), convert to/from the old enum-string
values in AccountGeneralPreferences. Everywhere else, use the key from
the scheme map, e.g. "anonymous http" instead of "ANON_HTTP". This
eliminates the special-case code when sending RPCs from the client,
and avoids weird undocumented behavior in the set preferences REST API
call.

Change-Id: I3e2397d8dfa15d20329cc83e1e3fe069c8e021c4
This commit is contained in:
Dave Borowitz 2015-09-03 15:27:19 -04:00
parent ae8f44a50a
commit 0a741dd21c
12 changed files with 140 additions and 113 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/AccountPreferencesInfo.java
View File

@ -21,7 +21,6 @@ import com.google.gerrit.reviewdb.client.AccountGeneralPreferences;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DateFormat;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DiffView;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadCommand;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.ReviewCategoryStrategy;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.TimeFormat;
import com.google.gwt.core.client.JavaScriptObject;
@ -73,11 +72,7 @@ public class AccountPreferencesInfo extends JavaScriptObject {
public final native boolean useFlashClipboard()
/*-{ return this.use_flash_clipboard || false }-*/;
public final DownloadScheme downloadScheme() {
String s = downloadSchemeRaw();
return s != null ? DownloadScheme.valueOf(s) : null;
}
private final native String downloadSchemeRaw()
public final native String downloadScheme()
/*-{ return this.download_scheme }-*/;
public final DownloadCommand downloadCommand() {
@ -142,10 +137,7 @@ public class AccountPreferencesInfo extends JavaScriptObject {
public final native void useFlashClipboard(boolean u)
/*-{ this.use_flash_clipboard = u }-*/;
public final void downloadScheme(DownloadScheme d) {
downloadSchemeRaw(d != null ? d.toString() : null);
}
private final native void downloadSchemeRaw(String d)
public final native void downloadScheme(String d)
/*-{ this.download_scheme = d }-*/;
public final void downloadCommand(DownloadCommand d) {

49
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/DownloadBox.java
View File

@ -26,7 +26,6 @@ import com.google.gerrit.client.rpc.NativeMap;
import com.google.gerrit.client.rpc.Natives;
import com.google.gerrit.client.rpc.RestApi;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.event.dom.client.ChangeEvent;
@ -221,7 +220,7 @@ class DownloadBox extends VerticalPanel {
scheme.setVisible(false);
} else {
int select = 0;
String find = getUserPreference();
String find = Gerrit.getUserPreferences().downloadScheme();
if (find != null) {
for (int i = 0; i < scheme.getItemCount(); i++) {
if (find.equals(scheme.getValue(i))) {
@ -236,35 +235,13 @@ class DownloadBox extends VerticalPanel {
renderCommands();
}
private static String getUserPreference() {
DownloadScheme pref = Gerrit.getUserPreferences().downloadScheme();
if (pref != null) {
switch (pref) {
case ANON_GIT:
return "git";
case ANON_HTTP:
return "anonymous http";
case HTTP:
return "http";
case SSH:
return "ssh";
case REPO_DOWNLOAD:
return "repo";
default:
return null;
}
}
return null;
}
private void saveScheme() {
DownloadScheme scheme = getSelectedScheme();
String schemeStr = scheme.getValue(scheme.getSelectedIndex());
AccountPreferencesInfo prefs = Gerrit.getUserPreferences();
if (Gerrit.isSignedIn() && scheme != null
&& scheme != prefs.downloadScheme()) {
prefs.downloadScheme(scheme);
if (Gerrit.isSignedIn() && !schemeStr.equals(prefs.downloadScheme())) {
prefs.downloadScheme(schemeStr);
AccountPreferencesInfo in = AccountPreferencesInfo.create();
in.downloadScheme(scheme);
in.downloadScheme(schemeStr);
AccountApi.self().view("preferences")
.put(in, new AsyncCallback<JavaScriptObject>() {
@Override
@ -277,20 +254,4 @@ class DownloadBox extends VerticalPanel {
});
}
}
private DownloadScheme getSelectedScheme() {
String id = scheme.getValue(scheme.getSelectedIndex());
if ("git".equals(id)) {
return DownloadScheme.ANON_GIT;
} else if ("anonymous http".equals(id)) {
return DownloadScheme.ANON_HTTP;
} else if ("http".equals(id)) {
return DownloadScheme.HTTP;
} else if ("ssh".equals(id)) {
return DownloadScheme.SSH;
} else if ("repo".equals(id)) {
return DownloadScheme.REPO_DOWNLOAD;
}
return null;
}
}

32
gerrit-gwtui/src/main/java/com/google/gerrit/client/download/DownloadUrlLink.java
View File

@ -18,7 +18,7 @@ import com.google.gerrit.client.Gerrit;
import com.google.gerrit.client.account.AccountApi;
import com.google.gerrit.client.info.AccountPreferencesInfo;
import com.google.gerrit.client.info.DownloadInfo.DownloadSchemeInfo;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.CoreDownloadSchemes;
import com.google.gwt.aria.client.Roles;
import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.event.dom.client.ClickEvent;
@ -32,17 +32,15 @@ import java.util.List;
public class DownloadUrlLink extends Anchor implements ClickHandler {
private enum KnownScheme {
ANON_GIT(DownloadScheme.ANON_GIT, "git", Util.M.anonymousDownload("Git")),
ANON_HTTP(DownloadScheme.ANON_HTTP, "anonymous http", Util.M.anonymousDownload("HTTP")),
SSH(DownloadScheme.SSH, "ssh", "SSH"),
HTTP(DownloadScheme.HTTP, "http", "HTTP");
ANON_GIT(CoreDownloadSchemes.ANON_GIT, Util.M.anonymousDownload("Git")),
ANON_HTTP(CoreDownloadSchemes.ANON_HTTP, Util.M.anonymousDownload("HTTP")),
SSH(CoreDownloadSchemes.SSH, "SSH"),
HTTP(CoreDownloadSchemes.HTTP, "HTTP");
public final DownloadScheme downloadScheme;
public final String name;
public final String text;
private KnownScheme(DownloadScheme downloadScheme, String name, String text) {
this.downloadScheme = downloadScheme;
private KnownScheme(String name, String text) {
this.name = name;
this.text = text;
}
@ -69,9 +67,9 @@ public class DownloadUrlLink extends Anchor implements ClickHandler {
KnownScheme knownScheme = KnownScheme.get(s);
if (knownScheme != null) {
urls.add(new DownloadUrlLink(downloadPanel, scheme,
knownScheme.downloadScheme, knownScheme.text));
knownScheme.name, knownScheme.text));
} else {
urls.add(new DownloadUrlLink(downloadPanel, scheme, s));
urls.add(new DownloadUrlLink(downloadPanel, scheme, s, s));
}
}
return urls;
@ -79,15 +77,10 @@ public class DownloadUrlLink extends Anchor implements ClickHandler {
private final DownloadPanel downloadPanel;
private final DownloadSchemeInfo schemeInfo;
private final DownloadScheme scheme;
private final String scheme;
public DownloadUrlLink(DownloadPanel downloadPanel,
DownloadSchemeInfo schemeInfo, String text) {
this(downloadPanel, schemeInfo, null, text);
}
public DownloadUrlLink(DownloadPanel downloadPanel,
DownloadSchemeInfo schemeInfo, DownloadScheme urlType, String text) {
DownloadSchemeInfo schemeInfo, String urlType, String text) {
super(text);
setStyleName(Gerrit.RESOURCES.css().downloadLink());
Roles.getTabRole().set(getElement());
@ -98,7 +91,7 @@ public class DownloadUrlLink extends Anchor implements ClickHandler {
this.scheme = urlType;
}
public DownloadScheme getUrlType() {
public String getUrlType() {
return scheme;
}
@ -110,8 +103,7 @@ public class DownloadUrlLink extends Anchor implements ClickHandler {
select();
AccountPreferencesInfo prefs = Gerrit.getUserPreferences();
if (Gerrit.isSignedIn() && scheme != null
&& scheme != prefs.downloadScheme()) {
if (Gerrit.isSignedIn() && !scheme.equals(prefs.downloadScheme())) {
prefs.downloadScheme(scheme);
AccountPreferencesInfo in = AccountPreferencesInfo.create();
in.downloadScheme(scheme);

5
gerrit-gwtui/src/main/java/com/google/gerrit/client/download/DownloadUrlPanel.java
View File

@ -15,7 +15,6 @@
package com.google.gerrit.client.download;
import com.google.gerrit.client.Gerrit;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences;
import com.google.gwt.aria.client.Roles;
import com.google.gwt.user.client.ui.FlowPanel;
import com.google.gwt.user.client.ui.Widget;
@ -33,7 +32,7 @@ public class DownloadUrlPanel extends FlowPanel {
return getWidgetCount() == 0;
}
public void select(AccountGeneralPreferences.DownloadScheme urlType) {
public void select(String urlType) {
DownloadUrlLink first = null;
for (Widget w : this) {
@ -42,7 +41,7 @@ public class DownloadUrlPanel extends FlowPanel {
if (first == null) {
first = d;
}
if (d.getUrlType() == urlType) {
if (d.getUrlType().equals(urlType)) {
d.select();
return;
}

6
gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpModule.java
View File

@ -14,7 +14,7 @@
package com.google.gerrit.httpd;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.CoreDownloadSchemes;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.config.DownloadConfig;
import com.google.inject.Inject;
@ -55,7 +55,7 @@ public class GitOverHttpModule extends ServletModule {
}
private boolean isHttpEnabled(){
return downloadConfig.getDownloadSchemes().contains(DownloadScheme.ANON_HTTP)
|| downloadConfig.getDownloadSchemes().contains(DownloadScheme.HTTP);
return downloadConfig.getDownloadSchemes().contains(CoreDownloadSchemes.ANON_HTTP)
|| downloadConfig.getDownloadSchemes().contains(CoreDownloadSchemes.HTTP);
}
}

53
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountGeneralPreferences.java
View File

@ -25,11 +25,6 @@ public final class AccountGeneralPreferences {
/** Valid choices for the page size. */
public static final short[] PAGESIZE_CHOICES = {10, 25, 50, 100};
/** Preferred scheme type to download a change. */
public static enum DownloadScheme {
ANON_GIT, ANON_HTTP, HTTP, SSH, REPO_DOWNLOAD
}
/** Preferred method to download a change. */
public static enum DownloadCommand {
REPO_DOWNLOAD, PULL, CHECKOUT, CHERRY_PICK, FORMAT_PATCH
@ -187,19 +182,49 @@ public final class AccountGeneralPreferences {
useFlashClipboard = b;
}
public DownloadScheme getDownloadUrl() {
if (downloadUrl == null) {
return null;
public String getDownloadUrl() {
// Translate from legacy enum names to modern display names. (May be removed
// if accompanied by a 2-phase schema upgrade.)
if (downloadUrl != null) {
switch (downloadUrl) {
case "ANON_GIT":
return CoreDownloadSchemes.ANON_GIT;
case "ANON_HTTP":
return CoreDownloadSchemes.ANON_HTTP;
case "HTTP":
return CoreDownloadSchemes.HTTP;
case "SSH":
return CoreDownloadSchemes.SSH;
case "REPO_DOWNLOAD":
return CoreDownloadSchemes.REPO_DOWNLOAD;
}
}
return DownloadScheme.valueOf(downloadUrl);
return downloadUrl;
}
public void setDownloadUrl(DownloadScheme url) {
if (url != null) {
downloadUrl = url.name();
} else {
downloadUrl = null;
public void setDownloadUrl(String url) {
// Translate from modern display names to legacy enum names. (May be removed
// if accompanied by a 2-phase schema upgrade.)
if (downloadUrl != null) {
switch (url) {
case CoreDownloadSchemes.ANON_GIT:
url = "ANON_GIT";
break;
case CoreDownloadSchemes.ANON_HTTP:
url = "ANON_HTTP";
break;
case CoreDownloadSchemes.HTTP:
url = "HTTP";
break;
case CoreDownloadSchemes.SSH:
url = "SSH";
break;
case CoreDownloadSchemes.REPO_DOWNLOAD:
url = "REPO_DOWNLOAD";
break;
}
}
downloadUrl = url;
}
public DownloadCommand getDownloadCommand() {

30
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/CoreDownloadSchemes.java Normal file
View File

@ -0,0 +1,30 @@
// Copyright (C) 2015 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.reviewdb.client;
/**
* Download scheme string constants supported by the download-commands core
* plugin.
*/
public class CoreDownloadSchemes {
public static final String ANON_GIT = "git";
public static final String ANON_HTTP = "anonymous http";
public static final String HTTP = "http";
public static final String SSH = "ssh";
public static final String REPO_DOWNLOAD = "repo";
private CoreDownloadSchemes() {
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/account/GetPreferences.java
View File

@ -24,7 +24,6 @@ import com.google.gerrit.reviewdb.client.AccountGeneralPreferences;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DateFormat;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DiffView;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadCommand;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.ReviewCategoryStrategy;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.TimeFormat;
import com.google.gerrit.reviewdb.server.ReviewDb;
@ -103,7 +102,7 @@ public class GetPreferences implements RestReadView<AccountResource> {
Short changesPerPage;
Boolean showSiteHeader;
Boolean useFlashClipboard;
DownloadScheme downloadScheme;
String downloadScheme;
DownloadCommand downloadCommand;
Boolean copySelfOnEmail;
DateFormat dateFormat;

3
gerrit-server/src/main/java/com/google/gerrit/server/account/SetPreferences.java
View File

@ -32,7 +32,6 @@ import com.google.gerrit.reviewdb.client.AccountGeneralPreferences;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DateFormat;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DiffView;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadCommand;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.ReviewCategoryStrategy;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.TimeFormat;
import com.google.gerrit.reviewdb.server.ReviewDb;
@ -60,7 +59,7 @@ public class SetPreferences implements RestModifyView<AccountResource, Input> {
public Short changesPerPage;
public Boolean showSiteHeader;
public Boolean useFlashClipboard;
public DownloadScheme downloadScheme;
public String downloadScheme;
public DownloadCommand downloadCommand;
public Boolean copySelfOnEmail;
public DateFormat dateFormat;

54
gerrit-server/src/main/java/com/google/gerrit/server/config/DownloadConfig.java
View File

@ -16,35 +16,50 @@ package com.google.gerrit.server.config;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadCommand;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.CoreDownloadSchemes;
import com.google.gerrit.server.change.ArchiveFormat;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.Config;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.ArrayList;
import java.util.EnumSet;
import java.util.List;
/** Download protocol from {@code gerrit.config}. */
/**
* Download protocol from {@code gerrit.config}.
* <p>
* Only used to configure the built-in set of schemes and commands in the core
* download-commands plugin; not used by other plugins.
*/
@Singleton
public class DownloadConfig {
private final ImmutableSet<DownloadScheme> downloadSchemes;
private final ImmutableSet<String> downloadSchemes;
private final ImmutableSet<DownloadCommand> downloadCommands;
private final ImmutableSet<ArchiveFormat> archiveFormats;
@Inject
DownloadConfig(@GerritServerConfig final Config cfg) {
List<DownloadScheme> allSchemes =
ConfigUtil.getEnumList(cfg, "download", null, "scheme",
DownloadScheme.values(), null);
if (isOnlyNull(allSchemes)) {
String[] allSchemes = cfg.getStringList("download", null, "scheme");
if (allSchemes.length == 0) {
downloadSchemes = ImmutableSet.of(
DownloadScheme.SSH,
DownloadScheme.HTTP,
DownloadScheme.ANON_HTTP);
CoreDownloadSchemes.SSH,
CoreDownloadSchemes.HTTP,
CoreDownloadSchemes.ANON_HTTP);
} else {
downloadSchemes = ImmutableSet.copyOf(allSchemes);
List<String> normalized = new ArrayList<>(allSchemes.length);
for (String s : allSchemes) {
String core = toCoreScheme(s);
if (core == null) {
throw new IllegalArgumentException(
"not a core download scheme: " + s);
}
normalized.add(core);
}
downloadSchemes = ImmutableSet.copyOf(normalized);
}
DownloadCommand[] downloadCommandValues = DownloadCommand.values();
@ -73,8 +88,23 @@ public class DownloadConfig {
return list.size() == 1 && list.get(0) == null;
}
private static String toCoreScheme(String s) {
try {
Field f = CoreDownloadSchemes.class.getField(s.toUpperCase());
int m = Modifier.PUBLIC | Modifier.STATIC | Modifier.FINAL;
if ((f.getModifiers() & m) == m && f.getType() == String.class) {
return (String) f.get(null);
} else {
return null;
}
} catch (NoSuchFieldException | SecurityException | IllegalArgumentException
| IllegalAccessException e) {
return null;
}
}
/** Scheme used to download. */
public ImmutableSet<DownloadScheme> getDownloadSchemes() {
public ImmutableSet<String> getDownloadSchemes() {
return downloadSchemes;
}

4
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java
View File

@ -14,7 +14,7 @@
package com.google.gerrit.sshd.commands;
import com.google.gerrit.reviewdb.client.AccountGeneralPreferences.DownloadScheme;
import com.google.gerrit.reviewdb.client.CoreDownloadSchemes;
import com.google.gerrit.server.config.DownloadConfig;
import com.google.gerrit.sshd.CommandModule;
import com.google.gerrit.sshd.CommandName;
@ -130,6 +130,6 @@ public class DefaultCommandModule extends CommandModule {
}
private boolean sshEnabled() {
return downloadConfig.getDownloadSchemes().contains(DownloadScheme.SSH);
return downloadConfig.getDownloadSchemes().contains(CoreDownloadSchemes.SSH);
}
}

2
plugins/download-commands

@ -1 +1 @@
Subproject commit 18b0f6d098a3bacaac022529140be85394549ff1
Subproject commit 6d4e0a45ad4d7faebc692e5f10e418cbfcf858cb