Index: /trunk/src/org/openstreetmap/josm/plugins/PluginHandler.java =================================================================== --- /trunk/src/org/openstreetmap/josm/plugins/PluginHandler.java (revision 13299) +++ /trunk/src/org/openstreetmap/josm/plugins/PluginHandler.java (revision 13300) @@ -1258,11 +1258,4 @@ File plugin = new File(filePath.substring(0, filePath.length() - 4)); String pluginName = updatedPlugin.getName().substring(0, updatedPlugin.getName().length() - 8); - if (plugin.exists() && !plugin.delete() && dowarn) { - Logging.warn(tr("Failed to delete outdated plugin ''{0}''.", plugin.toString())); - Logging.warn(tr("Failed to install already downloaded plugin ''{0}''. " + - "Skipping installation. JOSM is still going to load the old plugin version.", - pluginName)); - continue; - } try { // Check the plugin is a valid and accessible JAR file before installing it (fix #7754) @@ -1273,4 +1266,11 @@ plugin.toString(), updatedPlugin.toString(), e.getLocalizedMessage()), e); } + continue; + } + if (plugin.exists() && !plugin.delete() && dowarn) { + Logging.warn(tr("Failed to delete outdated plugin ''{0}''.", plugin.toString())); + Logging.warn(tr("Failed to install already downloaded plugin ''{0}''. " + + "Skipping installation. JOSM is still going to load the old plugin version.", + pluginName)); continue; } Index: /trunk/test/data/__files/plugin/corrupted_plugin.jar =================================================================== --- /trunk/test/data/__files/plugin/corrupted_plugin.jar (revision 13300) +++ /trunk/test/data/__files/plugin/corrupted_plugin.jar (revision 13300) @@ -0,0 +1,1 @@ +This is not a valid jar or zip file. Index: /trunk/test/data/plugin/corrupted_plugin.MANIFEST.MF =================================================================== --- /trunk/test/data/plugin/corrupted_plugin.MANIFEST.MF (revision 13300) +++ /trunk/test/data/plugin/corrupted_plugin.MANIFEST.MF (revision 13300) @@ -0,0 +1,12 @@ +Manifest-Version: 1.0 +Ant-Version: Apache Ant 1.10.1 +Created-By: 1.8.0_152-b16 (Oracle Corporation) +Plugin-Mainversion: 7654 +Plugin-Version: 5432 +Plugin-Class: org.openstreetmap.josm.plugin.corrupted.CorruptedPlugin +Plugin-Description: A plugin whose download gets corrupted. +Plugin-Date: 2017-06-01T12:34:56.789012Z +Author: Example Author +Plugin-Link: https://example.com/corrupted-plugin +Plugin-Icon: images/preferences/corrupted.png +Plugin-Canloadatruntime: false Index: /trunk/test/unit/org/openstreetmap/josm/TestUtils.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/TestUtils.java (revision 13299) +++ /trunk/test/unit/org/openstreetmap/josm/TestUtils.java (revision 13300) @@ -188,4 +188,20 @@ /** + * Sets a private static field value. + * @param cls object class + * @param fieldName private field name + * @param value replacement value + * @throws ReflectiveOperationException if a reflection operation error occurs + */ + public static void setPrivateStaticField(Class cls, String fieldName, final Object value) throws ReflectiveOperationException { + Field f = cls.getDeclaredField(fieldName); + AccessController.doPrivileged((PrivilegedAction) () -> { + f.setAccessible(true); + return null; + }); + f.set(null, value); + } + + /** * Returns an instance of {@link AbstractProgressMonitor} which keeps track of the monitor state, * but does not show the progress. Index: /trunk/test/unit/org/openstreetmap/josm/actions/downloadtasks/AbstractDownloadTaskTestParent.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/actions/downloadtasks/AbstractDownloadTaskTestParent.java (revision 13299) +++ /trunk/test/unit/org/openstreetmap/josm/actions/downloadtasks/AbstractDownloadTaskTestParent.java (revision 13300) @@ -40,4 +40,13 @@ /** + * Returns the {@code Content-Type} with which to serve the file referenced + * by {@link #getRemoteFile()} + * @return the {@code Content-Type} string for file {@link #getRemoteFile()} + */ + protected String getRemoteContentType() { + return "text/xml"; + } + + /** * Returns the http URL to remote test file to download. * @return the http URL to remote test file to download @@ -54,5 +63,5 @@ .willReturn(aResponse() .withStatus(200) - .withHeader("Content-Type", "text/xml") + .withHeader("Content-Type", getRemoteContentType()) .withBodyFile(getRemoteFile()))); } Index: /trunk/test/unit/org/openstreetmap/josm/actions/downloadtasks/PluginDownloadTaskTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/actions/downloadtasks/PluginDownloadTaskTest.java (revision 13300) +++ /trunk/test/unit/org/openstreetmap/josm/actions/downloadtasks/PluginDownloadTaskTest.java (revision 13300) @@ -0,0 +1,163 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.actions.downloadtasks; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; +import java.util.Collections; + +import org.junit.Rule; +import org.junit.Test; +import org.openstreetmap.josm.Main; +import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.gui.progress.NullProgressMonitor; +import org.openstreetmap.josm.plugins.PluginDownloadTask; +import org.openstreetmap.josm.plugins.PluginInformation; +import org.openstreetmap.josm.testutils.JOSMTestRules; + +import com.google.common.io.ByteStreams; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + +/** + * Unit tests for class {@link PluginDownloadTask}. + */ +public class PluginDownloadTaskTest extends AbstractDownloadTaskTestParent { + protected String pluginPath; + + /** + * Setup test. + */ + @Rule + @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") + public JOSMTestRules test = new JOSMTestRules().https().assumeRevision( + "Revision: 8000\n" + ).preferences(); + + @Override + protected String getRemoteContentType() { + return "application/java-archive"; + } + + @Override + protected String getRemoteFile() { + return this.pluginPath; + } + + /** + * Test download task when updating a plugin that we already have in our plugins directory + * and the downloaded file is valid + * @throws Exception if an error occurs + */ + @Test + public void testUpdatePluginValid() throws Exception { + this.pluginPath = "plugin/dummy_plugin.jar"; + this.mockHttp(); + + final File srcPluginFile = new File( + new File(TestUtils.getTestDataRoot()), "__files/" + this.pluginPath + ); + final File pluginDir = Main.pref.getPluginsDirectory(); + final File pluginFile = new File(pluginDir, "dummy_plugin.jar"); + final File pluginFileNew = new File(pluginDir, "dummy_plugin.jar.new"); + + // put existing "plugin file" in place + pluginFile.getParentFile().mkdirs(); + final byte[] existingPluginContents = "Existing plugin contents 123".getBytes(); + try (FileOutputStream existingPluginOutputStream = new FileOutputStream(pluginFile)) { + existingPluginOutputStream.write(existingPluginContents); + } + + // get PluginInformation from jar file + final PluginInformation pluginInformation = new PluginInformation(srcPluginFile); + // ...and grafting on the downloadlink + pluginInformation.downloadlink = this.getRemoteFileUrl(); + + final PluginDownloadTask pluginDownloadTask = new PluginDownloadTask( + NullProgressMonitor.INSTANCE, + Collections.singletonList(pluginInformation), + null + ); + pluginDownloadTask.run(); + + // the ".jar.new" file should have been deleted + assertFalse(pluginFileNew.exists()); + // the ".jar" file should still exist + assertTrue(pluginFile.exists()); + try ( + FileInputStream pluginDirPluginStream = new FileInputStream(pluginFile); + FileInputStream srcPluginStream = new FileInputStream(srcPluginFile); + ) { + // and its contents should equal those that were served to the task + assertArrayEquals( + ByteStreams.toByteArray(pluginDirPluginStream), + ByteStreams.toByteArray(srcPluginStream) + ); + } + } + + /** + * Test download task when updating a plugin that we already have in our plugins directory + * and the downloaded file is not a valid jar file. + * @throws Exception if an error occurs + */ + @Test + public void testUpdatePluginCorrupt() throws Exception { + this.pluginPath = "plugin/corrupted_plugin.jar"; + this.mockHttp(); + + final File srcPluginFile = new File( + new File(TestUtils.getTestDataRoot()), "__files/" + this.pluginPath + ); + final File pluginDir = Main.pref.getPluginsDirectory(); + final File pluginFile = new File(pluginDir, "corrupted_plugin.jar"); + final File pluginFileNew = new File(pluginDir, "corrupted_plugin.jar.new"); + // have to store this manifest externally as it clearly can't be read from our corrupted plugin + final File pluginManifest = new File(TestUtils.getTestDataRoot(), "plugin/corrupted_plugin.MANIFEST.MF"); + + pluginFile.getParentFile().mkdirs(); + final byte[] existingPluginContents = "Existing plugin contents 123".getBytes(); + try (FileOutputStream existingPluginOutputStream = new FileOutputStream(pluginFile)) { + existingPluginOutputStream.write(existingPluginContents); + } + + try (FileInputStream manifestInputStream = new FileInputStream(pluginManifest)) { + final PluginInformation pluginInformation = new PluginInformation( + manifestInputStream, + "corrupted_plugin", + this.getRemoteFileUrl() + ); + final PluginDownloadTask pluginDownloadTask = new PluginDownloadTask( + NullProgressMonitor.INSTANCE, + Collections.singletonList(pluginInformation), + null + ); + pluginDownloadTask.run(); + } + + // the ".jar.new" file should exist, even though invalid + assertTrue(pluginFileNew.exists()); + // the ".jar" file should still exist + assertTrue(pluginFile.exists()); + try ( + FileInputStream pluginDirPluginNewStream = new FileInputStream(pluginFileNew); + FileInputStream pluginDirPluginStream = new FileInputStream(pluginFile); + FileInputStream srcPluginStream = new FileInputStream(srcPluginFile); + ) { + // the ".jar" file's contents should be as before + assertArrayEquals( + existingPluginContents, + ByteStreams.toByteArray(pluginDirPluginStream) + ); + // just assert that the "corrupt" jar file made it through in tact + assertArrayEquals( + ByteStreams.toByteArray(pluginDirPluginNewStream), + ByteStreams.toByteArray(srcPluginStream) + ); + } + } +} Index: /trunk/test/unit/org/openstreetmap/josm/testutils/JOSMTestRules.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/testutils/JOSMTestRules.java (revision 13299) +++ /trunk/test/unit/org/openstreetmap/josm/testutils/JOSMTestRules.java (revision 13300) @@ -3,4 +3,5 @@ import java.awt.Color; +import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; @@ -16,7 +17,9 @@ import org.openstreetmap.josm.JOSMFixture; import org.openstreetmap.josm.Main; +import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.actions.DeleteAction; import org.openstreetmap.josm.command.DeleteCommand; import org.openstreetmap.josm.data.UserIdentityManager; +import org.openstreetmap.josm.data.Version; import org.openstreetmap.josm.data.osm.User; import org.openstreetmap.josm.data.osm.event.SelectionEventManager; @@ -59,4 +62,6 @@ private String i18n = null; private TileSourceRule tileSourceRule; + private String assumeRevisionString; + private Version originalVersion; private boolean platform; private boolean useProjection; @@ -133,4 +138,14 @@ public JOSMTestRules platform() { platform = true; + return this; + } + + /** + * Mock this test's assumed JOSM version (as reported by {@link Version}). + * @param revisionProperties mock contents of JOSM's {@code REVISION} properties file + * @return this instance, for easy chaining + */ + public JOSMTestRules assumeRevision(final String revisionProperties) { + this.assumeRevisionString = revisionProperties; return this; } @@ -277,5 +292,5 @@ /** * Use the {@link Main#main}, {@code Main.contentPanePrivate}, {@code Main.mainPanel}, - * {@link Main#menu}, {@link Main#toolbar} global variables in this test. + * global variables in this test. * @return this instance, for easy chaining * @since 12557 @@ -285,4 +300,12 @@ main = true; return this; + } + + private static class MockVersion extends Version { + MockVersion(final String propertiesString) { + super.initFromRevisionInfo( + new ByteArrayInputStream(propertiesString.getBytes()) + ); + } } @@ -318,10 +341,17 @@ * Set up before running a test * @throws InitializationError If an error occured while creating the required environment. - */ - protected void before() throws InitializationError { + * @throws ReflectiveOperationException if a reflective access error occurs + */ + protected void before() throws InitializationError, ReflectiveOperationException { // Tests are running headless by default. System.setProperty("java.awt.headless", "true"); cleanUpFromJosmFixture(); + + if (this.assumeRevisionString != null) { + this.originalVersion = Version.getInstance(); + final Version replacementVersion = new MockVersion(this.assumeRevisionString); + TestUtils.setPrivateStaticField(Version.class, "instance", replacementVersion); + } Config.setPreferencesInstance(Main.pref); @@ -465,7 +495,8 @@ /** * Clean up after running a test + * @throws ReflectiveOperationException if a reflective access error occurs */ @SuppressFBWarnings("DM_GC") - protected void after() { + protected void after() throws ReflectiveOperationException { // Sync AWT Thread GuiHelper.runInEDTAndWait(new Runnable() { @@ -481,4 +512,9 @@ Main.pref.resetToInitialState(); Main.platform = null; + + if (this.assumeRevisionString != null && this.originalVersion != null) { + TestUtils.setPrivateStaticField(Version.class, "instance", this.originalVersion); + } + // Parts of JOSM uses weak references - destroy them. System.gc();