Merge pull request #6045 from peterrinehart/BACKLOG-46631

[BACKLOG-46631] Ensured that the list of plugin directories is sorted

Author: pentaho-whartman

extensions/src/main/java/org/pentaho/platform/plugin/services/pluginmgr/SystemPathXmlPluginProvider.java

@@ -21,6 +21,7 @@
 import org.dom4j.Document;
 import org.dom4j.Element;
 import org.dom4j.Node;
+import org.slf4j.LoggerFactory;
 import org.pentaho.platform.api.engine.IContentGeneratorInfo;
 import org.pentaho.platform.api.engine.IPentahoSession;
 import org.pentaho.platform.api.engine.IPlatformPlugin;
@@ -48,6 +49,7 @@
 import java.io.File;
 import java.io.FilenameFilter;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -64,6 +66,7 @@ public class SystemPathXmlPluginProvider implements IPluginProvider {
 
   public static final String CLASS_PROPERRTY = "class";
   private static final Pattern PLUGIN_DATE_STAMP_REGEX = Pattern.compile( "([\\w\\-]+)-2\\d{3}-[\\d\\-]+" );
+  private static final org.slf4j.Logger log = LoggerFactory.getLogger( SystemPathXmlPluginProvider.class );
 
   /**
    * Gets the list of plugins that this provider class has discovered.
@@ -83,6 +86,7 @@ public List<IPlatformPlugin> getPlugins( IPentahoSession session ) throws Platfo
         "PluginManager.ERROR_0004_CANNOT_FIND_SYSTEM_FOLDER" ) ); //$NON-NLS-1$
     }
     File[] kids = systemDir.listFiles();
+    Arrays.sort( kids );  // default sort is by name, which is what we want
     // look at each child to see if it is a folder
     for ( File kid : kids ) {
       if ( kid.isDirectory() ) {
@@ -102,8 +106,11 @@ public List<IPlatformPlugin> getPlugins( IPentahoSession session ) throws Platfo
     return Collections.unmodifiableList( plugins );
   }
 
+  @SuppressWarnings( "squid:S3776" ) // cannot break down into smaller methods without reducing readability
   protected void processDirectory( List<IPlatformPlugin> plugins, File folder, IPentahoSession session )
     throws PlatformPluginRegistrationException {
+
+    log.debug( "Processing plugin directory: {}", folder.getAbsolutePath() );
     // see if there is a plugin.xml file
     FilenameFilter filter = new NameFileFilter( "plugin.xml", IOCase.SENSITIVE ); //$NON-NLS-1$
     File[] kids = folder.listFiles( filter );
@@ -114,13 +121,15 @@ protected void processDirectory( List<IPlatformPlugin> plugins, File folder, IPe
     FilenameFilter deleteFilter = new NameFileFilter( ".plugin-manager-delete", IOCase.SENSITIVE ); //$NON-NLS-1$
     kids = folder.listFiles( deleteFilter );
     if ( kids != null && kids.length > 0 ) {
+      log.debug( "Deleting plugin directory marked for deletion: {}", folder.getAbsolutePath() );
       deleteFolder( folder );
       return;
     }
     // see if we should ignore this plugin because it is marked to be ignored
     FilenameFilter ignoreFilter = new NameFileFilter( ".kettle-ignore", IOCase.SENSITIVE ); //$NON-NLS-1$
     kids = folder.listFiles( ignoreFilter );
     if ( kids != null && kids.length > 0 ) {
+      log.debug( "Ignoring plugin directory marked to be ignored: {}", folder.getAbsolutePath() );
       return;
     }
 
@@ -144,7 +153,19 @@ protected void processDirectory( List<IPlatformPlugin> plugins, File folder, IPe
         // XML document can't be read. We'll just return a null document.
       }
       if ( doc != null ) {
-        plugins.add( createPlugin( doc, session, folder.getName(), hasLib ) );
+        PlatformPlugin newPlugin = createPlugin( doc, session, folder.getName(), hasLib );
+        // make sure we don't already have a plugin with this id
+        if ( plugins.stream().anyMatch( p -> p.getId().equals( newPlugin.getId() ) ) ) {
+          String msg =
+            Messages.getInstance().getErrorString(
+              "PluginManager.ERROR_0024_PLUGIN_ALREADY_LOADED_BY_SAME_NAME", newPlugin.getId(), folder //$NON-NLS-1$
+                .getAbsolutePath() );
+          Logger.error( getClass().toString(), msg );
+          PluginMessageLogger.add( msg );
+          log.warn( "Plugin with id {} already loaded, skipping plugin in folder {}", newPlugin.getId(), path );
+        } else {
+          plugins.add( newPlugin );
+        }
       }
     } catch ( Exception e ) {
       throw new PlatformPluginRegistrationException( Messages.getInstance().getErrorString(
@@ -163,6 +184,7 @@ protected void processDirectory( List<IPlatformPlugin> plugins, File folder, IPe
    */
   private File cleanUpUninstalledPlugins( File folder, FilenameFilter deleteFilter, FilenameFilter ignoreFilter ) {
     // strip off any datestamp left from the plugin manager to leave only the base folder name
+    log.debug( "Checking for datestamp on plugin folder: {}", folder.getAbsolutePath() );
     String newFolderName = stripDateStampFromFolderName( folder.getName() );
     if ( !folder.getName().equals( newFolderName ) ) {
       // The plugin folder had a date stamp on the end of the name which was removed.
@@ -178,6 +200,7 @@ private File cleanUpUninstalledPlugins( File folder, FilenameFilter deleteFilter
         File[] deleteFiles = oldPluginFolder.listFiles( deleteFilter );
         if ( null != ignoreFiles && ignoreFiles.length == 1 && null != deleteFiles && deleteFiles.length == 1 ) {
           // we can and should delete this folder before we rename the other one
+          log.debug( "Deleting old plugin folder marked for deletion: {}", oldPluginFolder.getAbsolutePath() );
           deleteFolder( oldPluginFolder );
           canRenameFolder = true;
         } else {
@@ -208,6 +231,7 @@ private void deleteFolder( File folder ) {
     // delete the folder and its contents
     try {
       FileUtils.deleteDirectory( folder );
+      log.debug( "Deleted plugin directory: {}", folder.getAbsolutePath() );
       String msg = Messages.getInstance().getString(
         "PluginManager.PLUGIN_FOLDER_DELETED", folder.getAbsolutePath() );
       Logger.info( getClass().toString(), msg );

extensions/src/test/java/org/pentaho/platform/plugin/services/pluginmgr/SystemPathXmlPluginProviderTest.java

@@ -325,4 +325,33 @@ public void testProcessDirectory_renameFolderWithDateStamp() throws Exception {
       assertEquals( 1, plugins.size() );
     }
   }
+
+  /*
+   * This test forces the condition where a plugin with a given ID already exists in the plugins list.
+   * It verifies that the plugin manager does not add a duplicate plugin and logs an appropriate message.
+   */
+  @Test
+  public void testProcessDirectory_duplicatePluginId() throws Exception {
+    File pluginDir = tempFolder.newFolder( "duplicatePlugin" );
+    File pluginXml = new File( pluginDir, "plugin.xml" );
+    pluginXml.createNewFile();
+    List<IPlatformPlugin> plugins = new ArrayList<>();
+    IPentahoSession session = Mockito.mock( IPentahoSession.class );
+    // Add a plugin with the same ID to the list
+    IPlatformPlugin existingPlugin = Mockito.mock( IPlatformPlugin.class );
+    Mockito.when( existingPlugin.getId() ).thenReturn( "duplicatePlugin" );
+    plugins.add( existingPlugin );
+    // statically mock the ActionSequenceResource to always return an inputStream to the plugin.xml file
+    try ( MockedStatic<ActionSequenceResource> mockedStatic =
+      Mockito.mockStatic( ActionSequenceResource.class ) ) {
+      String pluginXmlText = "<plugin title=\"duplicatePlugin\" name=\"duplicatePlugin\"></plugin>";
+      // create an input stream from the pluginXmlText
+      mockedStatic.when( () -> ActionSequenceResource.getInputStream( anyString(), any() ) )
+        .thenReturn( new ByteArrayInputStream( pluginXmlText.getBytes() ) );
+      systemPathXmlPluginProvider.processDirectory( plugins, pluginDir, session );
+      // Verify that the plugin was not added again
+      assertEquals( 1, plugins.size() );
+      assertTrue( PluginMessageLogger.getAll().stream().anyMatch( msg -> msg.contains( "A plugin has already been registered by name duplicatePlugin" ) ) );
+    }
+  }
 }