From 955a98f7af541cc10674defeca53c4870ca8f441 Mon Sep 17 00:00:00 2001 From: Jason Plurad Date: Fri, 16 Oct 2015 10:27:51 -0400 Subject: [PATCH 1/2] TINKERPOP3-858 Cleanup after failed plugin install --- CHANGELOG.asciidoc | 2 ++ .../gremlin/console/commands/InstallCommand.groovy | 6 ++++++ .../gremlin/groovy/util/DependencyGrabber.groovy | 13 +++++++++++++ .../gremlin/server/util/GremlinServerInstall.java | 8 ++++++-- 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index ab8d3c9b2d2..c30b76e6734 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -25,6 +25,8 @@ image::https://raw.githubusercontent.com/apache/incubator-tinkerpop/master/docs/ TinkerPop 3.0.2 (NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Cleaned up `ext` directory when plugin installation fails for `gremlin-server` and `gremlin-console`. +* Fixed issues in `gremlin-server` when configured for HTTP basic authentication. * Made `BulkLoaderVertexProgram` work for any persistent TP3-supporting graph (input and output). * `TreeSideEffectStep` now implements `PathProcessor` which fixed a `ComputerVerificationStrategy` issue. * Added a shell script that verifies source and binary distributions. diff --git a/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy index a2efe2abd59..2bc7a60d257 100644 --- a/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy +++ b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy @@ -53,6 +53,12 @@ class InstallCommand extends CommandSupport { final def pluginsThatNeedRestart = grabDeps(dep) return "Loaded: " + arguments + (pluginsThatNeedRestart.size() == 0 ? "" : " - restart the console to use $pluginsThatNeedRestart") } catch (Exception ex) { + if (!(ex instanceof IllegalStateException)) { + // IllegalStateException is thrown if a module with the same name is already installed. + final def uninstall = new UninstallCommand(shell, mediator) + final List module = Collections.singletonList(artifact.getArtifact()) + uninstall.execute(module) + } return ex.message } } diff --git a/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy b/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy index f8065d7349a..e17c4a63ce6 100644 --- a/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy +++ b/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy @@ -47,6 +47,19 @@ class DependencyGrabber { this.extensionDirectory = extensionDirectory } + def String deleteDependenciesFromPath(final Artifact artifact) { + final def dep = makeDepsMap(artifact) + final String extClassPath = getPathFromDependency(dep) + final File f = new File(extClassPath) + if (!f.exists()) { + return "There is no module with the name ${dep.module} to remove - $extClassPath" + } + else { + f.deleteDir() + return "Uninstalled ${dep.module}" + } + } + def Set copyDependenciesToPath(final Artifact artifact) { final def dep = makeDepsMap(artifact) final String extClassPath = getPathFromDependency(dep) diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java index 03317d340f4..ffc8377d480 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java @@ -32,11 +32,15 @@ public static void main(final String[] arguments) { if (arguments.length != 3) { System.out.println("Usage: "); } else { + final Artifact artifact = new Artifact(arguments[0], arguments[1], arguments[2]); + final DependencyGrabber grabber = new DependencyGrabber(dummyClassLoader, getExtensionPath()); try { - final Artifact artifact = new Artifact(arguments[0], arguments[1], arguments[2]); - final DependencyGrabber grabber = new DependencyGrabber(dummyClassLoader, getExtensionPath()); grabber.copyDependenciesToPath(artifact); } catch (Exception iae) { + if (!(iae instanceof IllegalStateException)) { + // IllegalStateException is thrown if a module with the same name is already installed. + grabber.deleteDependenciesFromPath(artifact); + } System.out.println(String.format("Could not install the dependency: %s", iae.getMessage())); iae.printStackTrace(); } From 729396db8811a98c8b10f540ff45e6cf5f1dcd0c Mon Sep 17 00:00:00 2001 From: Jason Plurad Date: Sat, 17 Oct 2015 18:15:34 -0400 Subject: [PATCH 2/2] added dependency grabber test --- CHANGELOG.asciidoc | 2 +- .../console/commands/InstallCommand.groovy | 6 -- .../groovy/util/DependencyGrabber.groovy | 62 ++++++++----- .../groovy/util/DependencyGrabberTest.java | 90 +++++++++++++++++++ .../server/util/GremlinServerInstall.java | 8 +- 5 files changed, 131 insertions(+), 37 deletions(-) create mode 100644 gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabberTest.java diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index c30b76e6734..b60c9268cdf 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -25,7 +25,7 @@ image::https://raw.githubusercontent.com/apache/incubator-tinkerpop/master/docs/ TinkerPop 3.0.2 (NOT OFFICIALLY RELEASED YET) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -* Cleaned up `ext` directory when plugin installation fails for `gremlin-server` and `gremlin-console`. +* Cleaned up `ext/` directory when plugin installation fails for `gremlin-server` and `gremlin-console`. * Fixed issues in `gremlin-server` when configured for HTTP basic authentication. * Made `BulkLoaderVertexProgram` work for any persistent TP3-supporting graph (input and output). * `TreeSideEffectStep` now implements `PathProcessor` which fixed a `ComputerVerificationStrategy` issue. diff --git a/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy index 2bc7a60d257..a2efe2abd59 100644 --- a/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy +++ b/gremlin-console/src/main/groovy/org/apache/tinkerpop/gremlin/console/commands/InstallCommand.groovy @@ -53,12 +53,6 @@ class InstallCommand extends CommandSupport { final def pluginsThatNeedRestart = grabDeps(dep) return "Loaded: " + arguments + (pluginsThatNeedRestart.size() == 0 ? "" : " - restart the console to use $pluginsThatNeedRestart") } catch (Exception ex) { - if (!(ex instanceof IllegalStateException)) { - // IllegalStateException is thrown if a module with the same name is already installed. - final def uninstall = new UninstallCommand(shell, mediator) - final List module = Collections.singletonList(artifact.getArtifact()) - uninstall.execute(module) - } return ex.message } } diff --git a/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy b/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy index e17c4a63ce6..6b5242db80e 100644 --- a/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy +++ b/gremlin-groovy/src/main/groovy/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabber.groovy @@ -52,7 +52,7 @@ class DependencyGrabber { final String extClassPath = getPathFromDependency(dep) final File f = new File(extClassPath) if (!f.exists()) { - return "There is no module with the name ${dep.module} to remove - $extClassPath" + return "There is no module with the name ${dep.module} to remove - ${extClassPath}" } else { f.deleteDir() @@ -68,9 +68,16 @@ class DependencyGrabber { final File f = new File(extClassPath) if (f.exists()) throw new IllegalStateException("a module with the name ${dep.module} is already installed") - if (!f.mkdirs()) throw new IOException("could not create directory at ${f}") - if (!new File(extLibPath).mkdirs()) throw new IOException("could not create directory at ${extLibPath}") - if (!new File(extPluginPath).mkdirs()) throw new IOException("could not create directory at ${extPluginPath}") + + try { + if (!f.mkdirs()) throw new IOException("could not create directory at ${f}") + if (!new File(extLibPath).mkdirs()) throw new IOException("could not create directory at ${extLibPath}") + if (!new File(extPluginPath).mkdirs()) throw new IOException("could not create directory at ${extPluginPath}") + } catch (IOException ioe) { + // installation failed. make sure to cleanup directories. + deleteDependenciesFromPath(artifact) + throw ioe + } new File(extClassPath + fileSep + "plugin-info.txt").withWriter { out -> out << [artifact.group, artifact.artifact, artifact.version].join(":") } @@ -95,28 +102,35 @@ class DependencyGrabber { "structure can lead to unexpected behavior.") } - final def dependencyLocations = [] as Set - dependencyLocations.addAll(Grape.resolve([classLoader: this.classLoaderToUse], null, dep)) - - // for the "plugin" path ignore slf4j related jars. they are already in the path and will create duplicate - // bindings which generate annoying log messages that make you think stuff is wrong. also, don't bring - // over files that are already on the path. these dependencies will be part of the classpath - // - // additional dependencies are outside those pulled by grape and are defined in the manifest of the plugin jar. - // if a plugin uses that setting, it should force "restart" when the plugin is activated. right now, - // it is up to the plugin developer to enforce that setting. - dependencyLocations.collect(convertUriToPath(fs)) + try { + final def dependencyLocations = [] as Set + dependencyLocations.addAll(Grape.resolve([classLoader: this.classLoaderToUse], null, dep)) + + // for the "plugin" path ignore slf4j related jars. they are already in the path and will create duplicate + // bindings which generate annoying log messages that make you think stuff is wrong. also, don't bring + // over files that are already on the path. these dependencies will be part of the classpath + // + // additional dependencies are outside those pulled by grape and are defined in the manifest of the plugin jar. + // if a plugin uses that setting, it should force "restart" when the plugin is activated. right now, + // it is up to the plugin developer to enforce that setting. + dependencyLocations.collect(convertUriToPath(fs)) + .findAll { !(it.fileName.toFile().name ==~ /(slf4j|logback\-classic)-.*\.jar/) } + .findAll {!filesAlreadyInPath.collect { it.getFileName().toString() }.contains(it.fileName.toFile().name)} + .each(copyTo(targetPluginPath)) + getAdditionalDependencies(targetPluginPath, artifact).collect(convertUriToPath(fs)) .findAll { !(it.fileName.toFile().name ==~ /(slf4j|logback\-classic)-.*\.jar/) } - .findAll {!filesAlreadyInPath.collect { it.getFileName().toString() }.contains(it.fileName.toFile().name)} + .findAll { !filesAlreadyInPath.collect { it.getFileName().toString() }.contains(it.fileName.toFile().name)} .each(copyTo(targetPluginPath)) - getAdditionalDependencies(targetPluginPath, artifact).collect(convertUriToPath(fs)) - .findAll { !(it.fileName.toFile().name ==~ /(slf4j|logback\-classic)-.*\.jar/) } - .findAll { !filesAlreadyInPath.collect { it.getFileName().toString() }.contains(it.fileName.toFile().name)} - .each(copyTo(targetPluginPath)) - - // get dependencies for the lib path. the lib path should not filter out any jars - used for reference - dependencyLocations.collect(convertUriToPath(fs)).each(copyTo(targetLibPath)) - getAdditionalDependencies(targetLibPath, artifact).collect(convertUriToPath(fs)).each(copyTo(targetLibPath)) + + // get dependencies for the lib path. the lib path should not filter out any jars - used for reference + dependencyLocations.collect(convertUriToPath(fs)).each(copyTo(targetLibPath)) + getAdditionalDependencies(targetLibPath, artifact).collect(convertUriToPath(fs)).each(copyTo(targetLibPath)) + } + catch (Exception e) { + // installation failed. make sure to cleanup directories. + deleteDependenciesFromPath(artifact) + throw e + } // the ordering of jars seems to matter in some cases (e.g. neo4j). the plugin system allows the plugin // to place a Gremlin-Plugin-Paths entry in the jar manifest file to define where specific jar files should diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabberTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabberTest.java new file mode 100644 index 00000000000..b6ac3e24792 --- /dev/null +++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/util/DependencyGrabberTest.java @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.tinkerpop.gremlin.groovy.util; + +import groovy.lang.GroovyClassLoader; +import java.io.File; +import org.apache.commons.io.FileUtils; +import org.apache.tinkerpop.gremlin.groovy.plugin.Artifact; +import org.apache.tinkerpop.gremlin.util.Gremlin; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * @author Jason Plurad (http://github.com/pluradj) + */ +public class DependencyGrabberTest { + private static final GroovyClassLoader dummyClassLoader = new GroovyClassLoader(); + private static final File extTestDir = new File(System.getProperty("user.dir"), "/target/test-dep-ext"); + DependencyGrabber dg = null; + + @Before + public void setUp() { + FileUtils.deleteQuietly(extTestDir); + dg = new DependencyGrabber(dummyClassLoader, extTestDir.getAbsolutePath()); + } + + @After + public void tearDown() { + FileUtils.deleteQuietly(extTestDir); + } + + @Test + public void shouldInstallAndUninstallDependencies() { + final String pkg = "org.apache.tinkerpop"; + final String name = "gremlin-groovy"; + final String ver = Gremlin.version(); + final Artifact a = new Artifact(pkg, name, ver); + + // install the plugin + final File pluginDir = new File(extTestDir, name); + dg.copyDependenciesToPath(a); + assertTrue(pluginDir.exists()); + + // delete the plugin + dg.deleteDependenciesFromPath(a); + assertFalse(pluginDir.exists()); + } + + @Test(expected=IllegalStateException.class) + public void shouldThrowIllegalStateException() { + final String pkg = "org.apache.tinkerpop"; + final String name = "gremlin-groovy"; + final String ver = Gremlin.version(); + final Artifact a = new Artifact(pkg, name, ver); + + // install the plugin for the first time + dg.copyDependenciesToPath(a); + final File pluginDir = new File(extTestDir, name); + assertTrue(pluginDir.exists()); + + // attempt to install plugin a second time + try { + dg.copyDependenciesToPath(a); + } catch (IllegalStateException ise) { + // validate that the plugin dir wasn't deleted by accident + assertTrue(pluginDir.exists()); + // throw the IllegalStateException + throw ise; + } + } +} diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java index ffc8377d480..03317d340f4 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/GremlinServerInstall.java @@ -32,15 +32,11 @@ public static void main(final String[] arguments) { if (arguments.length != 3) { System.out.println("Usage: "); } else { - final Artifact artifact = new Artifact(arguments[0], arguments[1], arguments[2]); - final DependencyGrabber grabber = new DependencyGrabber(dummyClassLoader, getExtensionPath()); try { + final Artifact artifact = new Artifact(arguments[0], arguments[1], arguments[2]); + final DependencyGrabber grabber = new DependencyGrabber(dummyClassLoader, getExtensionPath()); grabber.copyDependenciesToPath(artifact); } catch (Exception iae) { - if (!(iae instanceof IllegalStateException)) { - // IllegalStateException is thrown if a module with the same name is already installed. - grabber.deleteDependenciesFromPath(artifact); - } System.out.println(String.format("Could not install the dependency: %s", iae.getMessage())); iae.printStackTrace(); }