Issue #3278 - ResourceCollection NPE

+ Adding tests + more NPE / ISE / IAE fixes

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java

@@ -84,8 +84,7 @@ public class ResourceCollection extends Resource
         _resources = list.toArray(new Resource[list.size()]);
         for(Resource r : _resources)
         {
-            if(!r.exists() || !r.isDirectory())
+            assertResourceValid(r);
-                throw new IllegalArgumentException(r + " is not an existing directory.");
         }
     }
     
@@ -98,17 +97,34 @@ public class ResourceCollection extends Resource
      */
     public ResourceCollection(String[] resources)
     {
-        _resources = new Resource[resources.length];
+        if(resources == null || resources.length ==0)
+        {
+            _resources = null;
+            return;
+        }
+
+        ArrayList<Resource> res = new ArrayList<>();
+
         try
         {
-            for(int i=0; i<resources.length; i++)
+            for(String strResource: resources)
             {
-                _resources[i] = Resource.newResource(resources[i]);
+                if(strResource == null || strResource.length() == 0)
-                if(!_resources[i].exists() || !_resources[i].isDirectory())
+                    continue; // skip null and empty string only (whitespace only is valid)
-                    throw new IllegalArgumentException(_resources[i] + " is not an existing directory.");
+                Resource resource = Resource.newResource(strResource);
+                assertResourceValid(resource);
+                res.add(resource);
             }
+
+            if (res.isEmpty())
+            {
+                _resources = null;
+                return;
             }
-        catch(IllegalArgumentException e)
+
+            _resources = res.toArray(new Resource[0]);
+        }
+        catch(RuntimeException e)
         {
             throw e;
         }
@@ -148,7 +164,31 @@ public class ResourceCollection extends Resource
      */
     public void setResources(Resource[] resources)
     {
-        _resources = resources != null ? resources : new Resource[0];
+        if (resources == null || resources.length == 0)
+        {
+            _resources = null;
+            return;
+        }
+
+        List<Resource> res = new ArrayList<>();
+        for (Resource resource : resources)
+        {
+            if (resource == null)
+            {
+                continue;
+            }
+
+            assertResourceValid(resource);
+            res.add(resource);
+        }
+
+        if (res.isEmpty())
+        {
+            _resources = null;
+            return;
+        }
+
+        _resources = res.toArray(new Resource[0]);
     }
 
     /* ------------------------------------------------------------ */
@@ -161,6 +201,9 @@ public class ResourceCollection extends Resource
      */
     public void setResourcesAsCSV(String csvResources)
     {
+        if(csvResources == null)
+            throw new IllegalArgumentException("CSV String is null");
+
         StringTokenizer tokenizer = new StringTokenizer(csvResources, ",;");
         int len = tokenizer.countTokens();
         if(len==0)
@@ -169,25 +212,38 @@ public class ResourceCollection extends Resource
                     " argument must be a string containing one or more comma-separated resource strings.");
         }
         
-        List<Resource> resources = new ArrayList<>();
+        List<Resource> res = new ArrayList<>();
         
         try
         {            
             while(tokenizer.hasMoreTokens())
             {
-                Resource resource = Resource.newResource(tokenizer.nextToken().trim());
+                String token = tokenizer.nextToken().trim();
-                if(!resource.exists() || !resource.isDirectory())
+                // TODO: we should not trim here, as whitespace is valid for paths
-                    LOG.warn(" !exist "+resource);
+                if (token == null || token.length() == 0)
-                else
+                    continue; // skip
-                    resources.add(resource);
+                Resource resource = Resource.newResource(token);
+                assertResourceValid(resource);
+                res.add(resource);
             }
+
+            if (res.isEmpty())
+            {
+                _resources = null;
+                return;
+            }
+
+            _resources = res.toArray(new Resource[0]);
+
+        }
+        catch (RuntimeException e)
+        {
+            throw e;
         }
         catch (Exception e)
         {
             throw new RuntimeException(e);
         }
-
-        _resources = resources.toArray(new Resource[resources.size()]);
     }
     
     /* ------------------------------------------------------------ */
@@ -198,8 +254,7 @@ public class ResourceCollection extends Resource
     @Override
     public Resource addPath(String path) throws IOException, MalformedURLException
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         if(path==null)
             throw new MalformedURLException();
@@ -227,7 +282,7 @@ public class ResourceCollection extends Resource
             if (r.exists() && r.isDirectory())
             {
                 if (resources==null)
-                    resources = new ArrayList<Resource>();
+                    resources = new ArrayList<>();
                     
                 if (resource!=null)
                 {
@@ -242,7 +297,7 @@ public class ResourceCollection extends Resource
         if (resource!=null)
             return resource;
         if (resources!=null)
-            return new ResourceCollection(resources.toArray(new Resource[resources.size()]));
+            return new ResourceCollection(resources.toArray(new Resource[0]));
         return null;
     }
 
@@ -255,6 +310,8 @@ public class ResourceCollection extends Resource
      */
     protected Object findResource(String path) throws IOException, MalformedURLException
     {
+        assertResourcesSet();
+
         Resource resource=null;
         ArrayList<Resource> resources = null;
         int i=0;
@@ -277,7 +334,7 @@ public class ResourceCollection extends Resource
             {
                 if (resource!=null)
                 {
-                    resources = new ArrayList<Resource>();
+                    resources = new ArrayList<>();
                     resources.add(resource);
                 }
                 resources.add(r);
@@ -302,8 +359,7 @@ public class ResourceCollection extends Resource
     @Override
     public boolean exists()
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         return true;
     }
@@ -312,8 +368,7 @@ public class ResourceCollection extends Resource
     @Override
     public File getFile() throws IOException
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         for(Resource r : _resources)
         {
@@ -328,8 +383,7 @@ public class ResourceCollection extends Resource
     @Override
     public InputStream getInputStream() throws IOException
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         for(Resource r : _resources)
         {
@@ -344,8 +398,7 @@ public class ResourceCollection extends Resource
     @Override 
     public ReadableByteChannel getReadableByteChannel() throws IOException
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         for(Resource r : _resources)
         {
@@ -360,8 +413,7 @@ public class ResourceCollection extends Resource
     @Override
     public String getName()
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         for(Resource r : _resources)
         {
@@ -376,8 +428,7 @@ public class ResourceCollection extends Resource
     @Override
     public URL getURL()
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         for(Resource r : _resources)
         {
@@ -392,8 +443,7 @@ public class ResourceCollection extends Resource
     @Override
     public boolean isDirectory()
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         return true;
     }
@@ -402,8 +452,7 @@ public class ResourceCollection extends Resource
     @Override
     public long lastModified()
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         for(Resource r : _resources)
         {
@@ -428,16 +477,15 @@ public class ResourceCollection extends Resource
     @Override
     public String[] list()
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
-        HashSet<String> set = new HashSet<String>();
+        HashSet<String> set = new HashSet<>();
         for(Resource r : _resources)
         {
             for(String s : r.list())
                 set.add(s);
         }
-        String[] result=set.toArray(new String[set.size()]);
+        String[] result=set.toArray(new String[0]);
         Arrays.sort(result);
         return result;
     }
@@ -446,8 +494,7 @@ public class ResourceCollection extends Resource
     @Override
     public void close()
     {
-        if(_resources==null || _resources.length==0)
+        assertResourcesSet();
-            throw new IllegalStateException("*resources* not set.");
 
         for(Resource r : _resources)
             r.close();
@@ -465,6 +512,8 @@ public class ResourceCollection extends Resource
     public void copyTo(File destination)
         throws IOException
     {
+        assertResourcesSet();
+
         for (int r=_resources.length;r-->0;)
             _resources[r].copyTo(destination);
     }
@@ -490,4 +539,19 @@ public class ResourceCollection extends Resource
         return false;
     }
 
+    private void assertResourcesSet()
+    {
+        if (_resources == null || _resources.length == 0)
+            throw new IllegalStateException("*resources* not set.");
+    }
+
+    private void assertResourceValid(Resource resource)
+    {
+        if (!resource.exists() || !resource.isDirectory())
+        {
+            throw new IllegalArgumentException(resource + " is not an existing directory.");
+        }
+    }
+
+
 }

jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceCollectionTest.java

@@ -18,18 +18,156 @@
 
 package org.eclipse.jetty.util.resource;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.InputStreamReader;
+import java.nio.file.Path;
 
+import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
+import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
+import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
 import org.eclipse.jetty.util.IO;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+@ExtendWith(WorkDirExtension.class)
 public class ResourceCollectionTest
 {
+    public WorkDir workdir;
+
+    @Test
+    public void testUnsetCollection_ThrowsISE()
+    {
+        ResourceCollection coll = new ResourceCollection();
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    @Test
+    public void testEmptyResourceArray_ThrowsISE()
+    {
+        ResourceCollection coll = new ResourceCollection(new Resource[0]);
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    @Test
+    public void testResourceArrayWithNull_ThrowsISE()
+    {
+        ResourceCollection coll = new ResourceCollection(new Resource[]{null});
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    @Test
+    public void testEmptyStringArray_ThrowsISE()
+    {
+        ResourceCollection coll = new ResourceCollection(new String[0]);
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    @Test
+    public void testStringArrayWithNull_ThrowsISE()
+    {
+        ResourceCollection coll = new ResourceCollection(new String[]{null});
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    @Test
+    public void testNullCsv_ThrowsIAE()
+    {
+        assertThrows(IllegalArgumentException.class, ()->{
+            String csv = null;
+            new ResourceCollection(csv); // throws IAE
+        });
+    }
+
+    @Test
+    public void testEmptyCsv_ThrowsIAE()
+    {
+        assertThrows(IllegalArgumentException.class, ()->{
+            String csv = "";
+            new ResourceCollection(csv); // throws IAE
+        });
+    }
+
+    @Test
+    public void testBlankCsv_ThrowsIAE()
+    {
+        assertThrows(IllegalArgumentException.class, () -> {
+            String csv = ",,,,";
+            new ResourceCollection(csv); // throws IAE
+        });
+    }
+
+    @Test
+    public void testSetResourceNull_ThrowsISE()
+    {
+        // Create a ResourceCollection with one valid entry
+        Path path = MavenTestingUtils.getTargetPath();
+        PathResource resource = new PathResource(path);
+        ResourceCollection coll = new ResourceCollection(resource);
+
+        // Reset collection to invalid state
+        coll.setResources(null);
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    @Test
+    public void testSetResourceEmpty_ThrowsISE()
+    {
+        // Create a ResourceCollection with one valid entry
+        Path path = MavenTestingUtils.getTargetPath();
+        PathResource resource = new PathResource(path);
+        ResourceCollection coll = new ResourceCollection(resource);
+
+        // Reset collection to invalid state
+        coll.setResources(new Resource[0]);
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    @Test
+    public void testSetResourceAllNulls_ThrowsISE()
+    {
+        // Create a ResourceCollection with one valid entry
+        Path path = MavenTestingUtils.getTargetPath();
+        PathResource resource = new PathResource(path);
+        ResourceCollection coll = new ResourceCollection(resource);
+
+        // Reset collection to invalid state
+        coll.setResources(new Resource[]{null,null,null});
+
+        assertThrowIllegalStateException(coll);
+    }
+
+    private void assertThrowIllegalStateException(ResourceCollection coll)
+    {
+        assertThrows(IllegalStateException.class, ()->coll.addPath("foo"));
+        assertThrows(IllegalStateException.class, ()->coll.findResource("bar"));
+        assertThrows(IllegalStateException.class, ()->coll.exists());
+        assertThrows(IllegalStateException.class, ()->coll.getFile());
+        assertThrows(IllegalStateException.class, ()->coll.getInputStream());
+        assertThrows(IllegalStateException.class, ()->coll.getReadableByteChannel());
+        assertThrows(IllegalStateException.class, ()->coll.getURL());
+        assertThrows(IllegalStateException.class, ()->coll.getName());
+        assertThrows(IllegalStateException.class, ()->coll.isDirectory());
+        assertThrows(IllegalStateException.class, ()->coll.lastModified());
+        assertThrows(IllegalStateException.class, ()->coll.list());
+        assertThrows(IllegalStateException.class, ()->coll.close());
+        assertThrows(IllegalStateException.class, ()->
+        {
+            Path destPath = workdir.getPathFile("bar");
+            coll.copyTo(destPath.toFile());
+        });
+    }
 
     @Test
     public void testMutlipleSources1() throws Exception