From c9b686056eeb344f21bdf259cc000ebd4fa8af65 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 30 Oct 2013 09:32:43 +0100 Subject: [PATCH 1/4] Explain limitation of storing plugin config in existing config files In the plugin documentation it is suggested to store plugin configuration parameters in a 'plugin' subsection in 'gerrit.config'/'project.config'. This approach has the limitation that the plugin configuration cannot have subsections and thus it is only suitable for plugins with a simple key-value pair configuration. Plugins that have a more complex configuration need to store their configuration in an own configuration file. Change-Id: Iac305e1bb451496034b06dbc762c8da08c97a59e Signed-off-by: Edwin Kempin --- Documentation/dev-plugins.txt | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index af087477dc..7a35115e1f 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -450,6 +450,17 @@ In Gerrit, global configuration is stored in the `gerrit.config` file. If a plugin needs global configuration, this configuration should be stored in a `plugin` subsection in the `gerrit.config` file. +This approach of storing the plugin configuration is only suitable for +plugins that have a simple configuration that only consists of +key-value pairs. With this approach it is not possible to have +subsections in the plugin configuration. Plugins that require a complex +configuration need to store their configuration in their own +configuration file where they can make use of subsections. On the other +hand storing the plugin configuration in a 'plugin' subsection in the +`gerrit.config` file has the advantage that administrators have all +configuration parameters in one file, instead of having one +configuration file per plugin. + To avoid conflicts with other plugins, it is recommended that plugins only use the `plugin` subsection with their own name. For example the `helloworld` plugin should store its configuration in the @@ -485,6 +496,17 @@ needs configuration on project level (e.g. to enable its functionality only for certain projects), this configuration should be stored in a `plugin` subsection in the project's `project.config` file. +This approach of storing the plugin configuration is only suitable for +plugins that have a simple configuration that only consists of +key-value pairs. With this approach it is not possible to have +subsections in the plugin configuration. Plugins that require a complex +configuration need to store their configuration in their own +configuration file where they can make use of subsections. On the other +hand storing the plugin configuration in a 'plugin' subsection in the +`project.config` file has the advantage that project owners have all +configuration parameters in one file, instead of having one +configuration file per plugin. + To avoid conflicts with other plugins, it is recommended that plugins only use the `plugin` subsection with their own name. For example the `helloworld` plugin should store its configuration in the From 122622d2236d12de9477a24fefe714f7414ae770 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 29 Oct 2013 16:45:44 +0100 Subject: [PATCH 2/4] Rename methods in PluginConfigFactory to be more explicit The methods in PluginConfigFactory that return the plugin configuration are renamed so that it is more explicit which plugin configuration is returned (the plugin configuration from 'gerrit.config' or from 'project.config'). This renaming breaks plugins that already use PluginConfigFactory, but since PluginConfigFactory was introduced quite late in the 2.8 development phase it should be OK to do the rename before 2.8 is released. In a future change it is planned to add support for plugin configuration that is stored in own configuration files (e.g. 'my-plugin.config'). For this it is required to add new accessor methods to PluginConfigFactory. The rename of the existing methods ensures that there will be no conflict with the names of the new accessor methods. In addition this change adds JavaDoc comments to the methods in PluginConfigFactory so that it is more clear which plugin configuration they return. Change-Id: I7c3e97d70cb16457cab6062e5d5126d8ed5de022 Signed-off-by: Edwin Kempin --- Documentation/dev-plugins.txt | 6 +- .../server/config/PluginConfigFactory.java | 86 +++++++++++++++++-- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 7a35115e1f..5e10a622d9 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -482,7 +482,7 @@ private com.google.gerrit.server.config.PluginConfigFactory cfg; [...] -String language = cfg.get("helloworld") +String language = cfg.getFromGerritConfig("helloworld") .getString("language", "English"); ---- @@ -528,7 +528,7 @@ private com.google.gerrit.server.config.PluginConfigFactory cfg; [...] -boolean enabled = cfg.get(project, "helloworld") +boolean enabled = cfg.getFromProjectConfig(project, "helloworld") .getBoolean("enabled", false); ---- @@ -542,7 +542,7 @@ private com.google.gerrit.server.config.PluginConfigFactory cfg; [...] -boolean enabled = cfg.getWithInheritance(project, "helloworld") +boolean enabled = cfg.getFromProjectConfigWithInheritance(project, "helloworld") .getBoolean("enabled", false); ---- diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfigFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfigFactory.java index 90b6d2f649..294d8a543b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfigFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfigFactory.java @@ -37,12 +37,49 @@ public class PluginConfigFactory { this.projectStateFactory = projectStateFactory; } - public PluginConfig get(String pluginName) { + /** + * Returns the configuration for the specified plugin that is stored in the + * 'gerrit.config' file. + * + * The returned plugin configuration provides access to all parameters of the + * 'gerrit.config' file that are set in the 'plugin' subsection of the + * specified plugin. + * + * E.g.: + * [plugin "my-plugin"] + * myKey = myValue + * + * @param pluginName the name of the plugin for which the configuration should + * be returned + * @return the plugin configuration from the 'gerrit.config' file + */ + public PluginConfig getFromGerritConfig(String pluginName) { return new PluginConfig(pluginName, cfg); } - public PluginConfig get(Project.NameKey projectName, String pluginName) - throws NoSuchProjectException { + /** + * Returns the configuration for the specified plugin that is stored in the + * 'project.config' file of the specified project. + * + * The returned plugin configuration provides access to all parameters of the + * 'project.config' file that are set in the 'plugin' subsection of the + * specified plugin. + * + * E.g.: + * [plugin "my-plugin"] + * myKey = myValue + * + * @param projectName the name of the project for which the plugin + * configuration should be returned + * @param pluginName the name of the plugin for which the configuration should + * be returned + * @return the plugin configuration from the 'project.config' file of the + * specified project + * @throws NoSuchProjectException thrown if the specified project does not + * exist + */ + public PluginConfig getFromProjectConfig(Project.NameKey projectName, + String pluginName) throws NoSuchProjectException { ProjectState projectState = projectCache.get(projectName); if (projectState == null) { throw new NoSuchProjectException(projectName); @@ -50,8 +87,45 @@ public class PluginConfigFactory { return projectState.getConfig().getPluginConfig(pluginName); } - public PluginConfig getWithInheritance(Project.NameKey projectName, - String pluginName) throws NoSuchProjectException { - return get(projectName, pluginName).withInheritance(projectStateFactory); + /** + * Returns the configuration for the specified plugin that is stored in the + * 'project.config' file of the specified project. Parameters which are not + * set in the 'project.config' of this project are inherited from the parent + * project's 'project.config' files. + * + * The returned plugin configuration provides access to all parameters of the + * 'project.config' file that are set in the 'plugin' subsection of the + * specified plugin. + * + * E.g.: + * child project: + * [plugin "my-plugin"] + * myKey = childValue + * + * parent project: + * [plugin "my-plugin"] + * myKey = parentValue + * anotherKey = someValue + * + * return: + * [plugin "my-plugin"] + * myKey = childValue + * anotherKey = someValue + * + * @param projectName the name of the project for which the plugin + * configuration should be returned + * @param pluginName the name of the plugin for which the configuration should + * be returned + * @return the plugin configuration from the 'project.config' file of the + * specified project with inherited non-set parameters from the + * parent projects + * @throws NoSuchProjectException thrown if the specified project does not + * exist + */ + public PluginConfig getFromProjectConfigWithInheritance( + Project.NameKey projectName, String pluginName) + throws NoSuchProjectException { + return getFromProjectConfig(projectName, pluginName).withInheritance( + projectStateFactory); } } From 8f5aa59368b501dc518da615921c4fd859cd12f1 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 29 Oct 2013 11:16:53 -0700 Subject: [PATCH 3/4] ChangeField: skip PatchSets with null revisions The revision field has notNull = false, so it may be null. Current usages always call setRevision() on new PatchSets, but old changes in the DB may be corrupt. Avoid an NPE when indexing these old changes. Change-Id: I7467e94bd1b5d1b77e29dd26925463152160512d (cherry picked from commit 849c5c8e00ed390af667a42fe387872e416c781a) --- .../main/java/com/google/gerrit/server/index/ChangeField.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java index a74ef01511..290bd3143c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java @@ -206,7 +206,9 @@ public class ChangeField { throws OrmException { Set revisions = Sets.newHashSet(); for (PatchSet ps : input.patches(args.db)) { - revisions.add(ps.getRevision().get()); + if (ps.getRevision() != null) { + revisions.add(ps.getRevision().get()); + } } return revisions; } From b1d685db437ea3b58f9066f6c696e27db2588bd3 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 29 Oct 2013 11:19:35 -0700 Subject: [PATCH 4/4] PatchListCacheImpl: explicitly check for null revision Change-Id: Ib88d104ebd288e16ee2499ca93cecd4a8b9e2b2a (cherry picked from commit 8909bc4c2c94ed80bf315a286aadaa56ec77ebd6) --- .../com/google/gerrit/server/patch/PatchListCacheImpl.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index 967e6a7cda..7b7c73108a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -90,6 +90,10 @@ public class PatchListCacheImpl implements PatchListCache { throws PatchListNotAvailableException { final Project.NameKey projectKey = change.getProject(); final ObjectId a = null; + if (patchSet.getRevision() == null) { + throw new PatchListNotAvailableException( + "revision is null for " + patchSet.getId()); + } final ObjectId b = ObjectId.fromString(patchSet.getRevision().get()); final Whitespace ws = Whitespace.IGNORE_NONE; return get(new PatchListKey(projectKey, a, b, ws));