Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not iterate through transient plugin dependencies if a newer dependency version is explicitly defined in the plugin list #102

Merged
merged 11 commits into from
Jun 16, 2020
Merged
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ jenkins_*.build
jenkins_*.changes
*.deb
*.pkg
*.zip
push-build.sh
war/node_modules/
war/yarn-error.log
Expand Down
6 changes: 6 additions & 0 deletions plugin-management-library/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@
<artifactId>json</artifactId>
<version>20180813</version>
</dependency>
<dependency>
<groupId>com.github.stefanbirkner</groupId>
<artifactId>system-rules</artifactId>
<version>1.19.0</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import hudson.util.VersionNumber;
import java.io.File;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import org.apache.commons.lang3.StringUtils;
Expand All @@ -15,6 +16,8 @@ public class Plugin implements Comparable<Plugin> {
private String url;
private File file;
private List<Plugin> dependencies;
//TODO(oleg_nenashev): better to use nullable API
private boolean dependenciesSpecified;
private Plugin parent;
private List<SecurityWarning> securityWarnings;
private boolean latest;
Expand Down Expand Up @@ -99,12 +102,27 @@ public String getGroupId() {

public void setDependencies(List<Plugin> dependencies) {
this.dependencies = dependencies;
this.dependenciesSpecified = true;
}

public Plugin withDependencies(List<Plugin> dependencies) {
this.setDependencies(dependencies);
return this;
}

public Plugin withoutDependencies() {
this.setDependencies(Collections.emptyList());
return this;
}

public List<Plugin> getDependencies() {
return dependencies;
}

public boolean isDependenciesSpecified() {
return dependenciesSpecified;
}

public void setParent(Plugin parent) {
this.parent = parent;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.regex.Matcher;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.annotation.CheckForNull;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.io.IOUtils;
Expand Down Expand Up @@ -108,6 +109,18 @@ public PluginManager(Config cfg) {
* the failed plugins
*/
public void start() {
start(true);
}

/**
* Drives the process to download plugins.
* Calls methods to find installed plugins, download plugins, and output the failed plugins.
*
* @param downloadUc {@code false} to disable Update Center and other external resources download.
* In such case the update center metadata should be provided by API.
* @since TODO
*/
public void start(boolean downloadUc) {
if (refDir.exists()) {
try {
FileUtils.deleteDirectory(refDir);
Expand All @@ -122,9 +135,11 @@ public void start() {
"at a time");
}

jenkinsVersion = getJenkinsVersionFromWar();
checkAndSetLatestUpdateCenter();
getUCJson();
if (downloadUc) {
jenkinsVersion = getJenkinsVersionFromWar();
checkAndSetLatestUpdateCenter();
getUCJson();
}
getSecurityWarnings();
showAllSecurityWarnings();
bundledPluginVersions = bundledPlugins();
Expand Down Expand Up @@ -465,11 +480,16 @@ public void downloadPlugins(List<Plugin> plugins) {
* @return set of all requested plugins and their recursive dependencies
*/
public Map<String, Plugin> findPluginsAndDependencies(List<Plugin> requestedPlugins) {
Map<String, Plugin> allPluginDependencies = new HashMap<>();
// Prepare the initial list by putting all explicitly requested plugins
Map<String, Plugin> topLevelDependencies = new HashMap<>();
for (Plugin requestedPlugin : requestedPlugins) {
topLevelDependencies.put(requestedPlugin.getName(), requestedPlugin);
}
Map<String, Plugin> allPluginDependencies = new HashMap<>(topLevelDependencies);

for (Plugin requestedPlugin : requestedPlugins) {
//for each requested plugin, find all the dependent plugins that will be downloaded (including requested plugin)
Map<String, Plugin> dependencies = resolveRecursiveDependencies(requestedPlugin);
Map<String, Plugin> dependencies = resolveRecursiveDependencies(requestedPlugin, topLevelDependencies);

for (Plugin dependentPlugin : dependencies.values()) {
String dependencyName = dependentPlugin.getName();
Expand Down Expand Up @@ -607,11 +627,17 @@ public JSONArray getPluginDependencyJsonArray(Plugin plugin, JSONObject ucJson)
}

/**
* Retrieves the latest available version of a specified plugin.
*
* @param pluginName the name of the plugin
* @return latest version of the specified plugin
* @throws IllegalStateException Update Center JSON has not been retrieved yet
*/
public VersionNumber getLatestPluginVersion(String pluginName) {
if (latestPlugins == null) {
throw new IllegalStateException("List of plugins is not available. Likely Update Center data has not been downloaded yet");
}

if (!latestPlugins.has(pluginName)) {
throw new PluginNotFoundException(String.format("Unable to find plugin %s in update center %s", pluginName,
jenkinsUcLatest));
Expand Down Expand Up @@ -768,13 +794,19 @@ public List<Plugin> resolveDirectDependencies(Plugin plugin) {

/**
* Finds all recursive dependencies for a given plugin. If the same plugin is required by different plugins, the
* highest required version will be taken
* highest required version will be taken.
*
* @param plugin to resolve dependencies for
* @return map of plugin names and plugins representing all of the dependencies of the requested plugin, including
* the requested plugin itself
*/
public Map<String, Plugin> resolveRecursiveDependencies(Plugin plugin) {
return resolveRecursiveDependencies(plugin, null);
}

// TODO(oleg_nenashev): This method is private, because it is only a partial fix for https://github.com/jenkinsci/plugin-installation-manager-tool/issues/101
// A full dependency graph resolution and removal of non-needed dependency trees is required
private Map<String, Plugin> resolveRecursiveDependencies(Plugin plugin, @CheckForNull Map<String, Plugin> topLevelDependencies) {
Deque<Plugin> queue = new LinkedList<>();
Map<String, Plugin> recursiveDependencies = new HashMap<>();
queue.add(plugin);
Expand All @@ -783,12 +815,31 @@ public Map<String, Plugin> resolveRecursiveDependencies(Plugin plugin) {
while (queue.size() != 0) {
Plugin dependency = queue.poll();

if (dependency.getDependencies().isEmpty()) {
if (!dependency.isDependenciesSpecified()) {
dependency.setDependencies(resolveDirectDependencies(dependency));
}

for (Plugin p : dependency.getDependencies()) {
String dependencyName = p.getName();
Plugin pinnedPlugin = topLevelDependencies != null ? topLevelDependencies.get(dependencyName) : null;

// See https://github.com/jenkinsci/plugin-installation-manager-tool/pull/102
if (pinnedPlugin != null) { // There is a top-level plugin with the same ID
if (pinnedPlugin.getVersion().isNewerThanOrEqualTo(p.getVersion())) {
if (verbose) {
logVerbose(String.format("Skipping dependency %s:%s and its sub-dependencies, because there is a higher version defined on the top level - %s:%s",
p.getName(), p.getVersion(), pinnedPlugin.getName(), pinnedPlugin.getVersion()));
}
continue;
} else {
String message = String.format("Plugin %s:%s depends on %s:%s, but there is an older version defined on the top level - %s:%s",
plugin.getName(), plugin.getVersion(), p.getName(), p.getVersion(), pinnedPlugin.getName(), pinnedPlugin.getVersion());
// TODO(oleg_nenashev): Should be an error by default, but it is not how the tests are written now
Copy link
Member Author

@oleg-nenashev oleg-nenashev Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO there should be an error by default, but it is not how the tests are written now

Skipping it for now, original test from @stopalopa will fail

// throw new PluginDependencyStrategyException(message);
logVerbose(message);
}
}

if (!recursiveDependencies.containsKey(dependencyName)) {
recursiveDependencies.put(dependencyName, p);
queue.add(p);
Expand Down Expand Up @@ -1175,7 +1226,7 @@ public void setPluginsToBeDownloaded(List<Plugin> pluginsToBeDownloaded) {
}

/**
* Outputs inforation to the console if verbose option was set to true
* Outputs information to the console if verbose option was set to true
*
* @param message informational string to output
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
package io.jenkins.tools.pluginmanager.impl;

import hudson.util.VersionNumber;
import io.jenkins.tools.pluginmanager.config.Config;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;
import org.apache.commons.io.IOUtils;
import org.json.JSONObject;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.contrib.java.lang.system.SystemOutRule;
import org.junit.rules.TemporaryFolder;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.not;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;

/**
* Tests for {@link PluginManager} which operate with real data.
* No mocks here.
*/
public class PluginManagerIntegrationTest {

static JSONObject latestUcJson;
static File pluginVersionsFile;

@ClassRule
public static TemporaryFolder tmp = new TemporaryFolder();
public static File jenkinsWar;
public static File cacheDir;
public static File pluginsDir;

@Rule
public final SystemOutRule systemOutRule = new SystemOutRule().enableLog();

// TODO: Convert to a rule
public interface Configurator {
void configure(Config.Builder configBuilder);
}

public static PluginManager initPluginManager(Configurator configurator) throws IOException {
Config.Builder configBuilder = Config.builder()
.withJenkinsWar(jenkinsWar.getAbsolutePath())
.withPluginDir(pluginsDir)
.withShowAvailableUpdates(true)
.withIsVerbose(true)
.withDoDownload(false);
configurator.configure(configBuilder);
Config config = configBuilder.build();

PluginManager pluginManager = new PluginManager(config);
pluginManager.setCm(new CacheManager(cacheDir.toPath(), true));
pluginManager.setJenkinsVersion(new VersionNumber("2.222.1"));
pluginManager.setLatestUcJson(latestUcJson);
pluginManager.setLatestUcPlugins(latestUcJson.getJSONObject("plugins"));
pluginManager.setPluginInfoJson(pluginManager.getJson(pluginVersionsFile.toURI().toURL(), "plugin-versions"));

return pluginManager;
}

private static void unzipResource(Class clazz, String resourceName, String fileName, File target) throws IOException {
final File archivePath = new File(tmp.getRoot(), target.toPath().getFileName() + ".zip");
try (InputStream archive = clazz.getResourceAsStream(resourceName)) {
if (archive == null) {
throw new IOException("Cannot find " + resourceName + " in " + clazz);
}
Files.copy(archive, archivePath.toPath());
}

try (ZipFile zip = new ZipFile(archivePath)){
ZipEntry entry = zip.getEntry(fileName);
if (entry == null) {
throw new IOException(String.format("Cannot find file %s within ZIP resource %s/%s", fileName, clazz.getName(), resourceName));
}
try(InputStream archive = zip.getInputStream(entry)) {
Files.copy(archive, target.toPath());
}
}
}

@BeforeClass
public static void setup() throws Exception {
File updatesJSON = new File(tmp.getRoot(), "updates.json");
unzipResource(PluginManagerIntegrationTest.class, "updates.zip", "updates.json", updatesJSON);
try (InputStream istream = new FileInputStream(updatesJSON)) {
latestUcJson = new JSONObject(IOUtils.toString(istream, StandardCharsets.UTF_8));
}

pluginVersionsFile = new File(tmp.getRoot(), "plugin-versions.json");
unzipResource(PluginManagerIntegrationTest.class, "plugin-versions.zip", "plugin-versions.json", pluginVersionsFile);

//TODO: Use real 2.222.1 war instead
jenkinsWar = new File(tmp.getRoot(), "jenkins.war");
try(InputStream war = PluginManagerIntegrationTest.class.getResourceAsStream("/bundledplugintest.war")) {
Files.copy(war, jenkinsWar.toPath());
}

cacheDir = tmp.newFolder("cache");
pluginsDir = tmp.newFolder("pluginsDir");
}

// https://github.com/jenkinsci/plugin-installation-manager-tool/issues/101
@Test
public void showAvailableUpdates_shouldNotFailOnUIThemes() throws IOException {
Plugin pluginDockerCommons = new Plugin("docker-commons", "1.16", null, null);
Plugin pluginYAD = new Plugin("yet-another-docker-plugin", "0.2.0", null, null);
Plugin pluginIconShim = new Plugin("icon-shim", "2.0.3", null, null);
PluginManager pluginManager = initPluginManager(
configBuilder -> configBuilder.withPlugins(Arrays.asList(pluginDockerCommons, pluginIconShim, pluginYAD)));

pluginManager.start(false);
assertThat(systemOutRule.getLog(), not(containsString("uithemes")));
}

@Test
public void findPluginsAndDependenciesTest() throws IOException {
Plugin plugin1 = new Plugin("plugin1", "1.0", null, null);
Plugin replaced = new Plugin("replaced", "1.0", null, null).withoutDependencies();
Plugin plugin2 = new Plugin("plugin2", "2.0", null, null);
Plugin plugin3 = new Plugin("plugin3", "3.0", null, null);

Plugin plugin1Dependency1 = new Plugin("plugin1Dependency1", "1.0.1", null, null).withoutDependencies();
Plugin plugin1Dependency2 = new Plugin("plugin1Dependency2", "1.0.2", null, null).withoutDependencies();
Plugin replaced1 = new Plugin("replaced", "1.0.1", null, null).withoutDependencies();
plugin1.setDependencies(Arrays.asList(plugin1Dependency1, plugin1Dependency2, replaced1));

Plugin plugin2Dependency1 = new Plugin("plugin2Dependency1", "2.0.1", null, null).withoutDependencies();
Plugin plugin2Dependency2 = new Plugin("plugin2Dependency2", "2.0.2", null, null).withoutDependencies();
Plugin replaced2 = new Plugin("replaced", "2.0.2", null, null).withoutDependencies();
Plugin replacedSecond = new Plugin("replaced2", "2.0.2", null, null).withoutDependencies();
plugin2.setDependencies(Arrays.asList(plugin2Dependency1, plugin2Dependency2, replaced2, replacedSecond));

Plugin plugin3Dependency1 = new Plugin("plugin3Dependency1", "3.0.1", null, null).withoutDependencies();
Plugin replacedSecond2 = new Plugin("replaced2", "3.2", null, null).withoutDependencies();
plugin3.setDependencies(Arrays.asList(plugin3Dependency1, replacedSecond2));

// Expected
List<Plugin> expectedPlugins = new ArrayList<>(Arrays.asList(plugin1, plugin1Dependency1, plugin1Dependency2,
plugin2Dependency1, plugin2, plugin2Dependency2, plugin3, plugin3Dependency1, replaced2, replacedSecond2));
Collections.sort(expectedPlugins);

// Actual
List<Plugin> requestedPlugins = new ArrayList<>(Arrays.asList(plugin1, plugin2, plugin3, replaced));
PluginManager pluginManager = initPluginManager(
configBuilder -> configBuilder.withPlugins(requestedPlugins));
List<Plugin> pluginsAndDependencies = new ArrayList<>(pluginManager.findPluginsAndDependencies(requestedPlugins).values());
Collections.sort(pluginsAndDependencies);

assertEquals(expectedPlugins, pluginsAndDependencies);
}
}
Loading