Modify include directory parsing to match Liberty (#426)

* Modify behavior for include directory parsing to match Liberty

* Add ability to track and verify error logs in tests. Verify invalid include dir error message

src/main/java/io/openliberty/tools/common/plugins/config/ServerConfigDocument.java

@@ -450,11 +450,8 @@ private static ArrayList<Document> getIncludeDocs(String loc) throws IOException
             // TODO handle ftp protocol
         } else {
             locFile = new File(loc);
-
             // check if absolute file
-            if (locFile.isAbsolute()) {
-                parseDocumentFromFileOrDirectory(locFile, docs);
-            } else {
+            if (!locFile.isAbsolute()) {
                 // check configDirectory first if exists
                 if (configDirectory != null && configDirectory.exists()) {
                     locFile = new File(configDirectory, loc);
@@ -463,8 +460,8 @@ private static ArrayList<Document> getIncludeDocs(String loc) throws IOException
                 if (locFile == null || !locFile.exists()) {
                     locFile = new File(getServerXML().getParentFile(), loc);
                 }
-                parseDocumentFromFileOrDirectory(locFile, docs);
             }
+            parseDocumentFromFileOrDirectory(locFile, loc, docs);
         }
 
         if (docs.isEmpty()) {
@@ -475,24 +472,36 @@ private static ArrayList<Document> getIncludeDocs(String loc) throws IOException
 
     /**
      * Parses file or directory for all xml documents, and adds to ArrayList<Document>
-     * @param file - file or directory to parse documents from
+     * @param f - file or directory to parse documents from
+     * @param locationString - String representation of filepath for f
      * @param docs - ArrayList to store parsed Documents.
      * @throws FileNotFoundException
      * @throws IOException
      * @throws SAXException
      */
-    private static void parseDocumentFromFileOrDirectory(File file, ArrayList<Document> docs) throws FileNotFoundException, IOException, SAXException {
+    private static void parseDocumentFromFileOrDirectory(File f, String locationString, ArrayList<Document> docs) throws FileNotFoundException, IOException, SAXException {
         Document doc = null;
-        if (file == null || !file.exists()) {
-            log.warn("Unable to parse from file: " + file.getCanonicalPath());
+        // Earlier call to VariableUtility.resolveVariables() already converts all \ to /
+        boolean isLibertyDirectory = locationString.endsWith("/");  // Liberty uses this to determine if directory. 
+
+        if (f == null || !f.exists()) {
+            log.warn("Unable to parse from file: " + f.getCanonicalPath());
             return;
         }
-        if (file.isFile()) {
-            doc = parseDocument(file);
-            docs.add(doc);
+        // If file mismatches Liberty definition of directory
+        if (f.isFile() && isLibertyDirectory) {
+            log.error("Path specified a directory, but resource exists as a file (path=" + locationString + ")");
+            return;
+        } else if (f.isDirectory() && !isLibertyDirectory) {
+            log.error("Path specified a file, but resource exists as a directory (path=" + locationString + ")");
+            return;
         }
-        if (file.isDirectory()) {
-            parseDocumentsInDirectory(file, docs);
+
+        if (f.isDirectory()) {
+            parseDocumentsInDirectory(f, docs);
+        } else {
+            doc = parseDocument(f);
+            docs.add(doc);
         }
     }
 

src/main/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtil.java

@@ -284,6 +284,18 @@ private Set<File> getAdditionalJsons() {
      */
     public abstract void error(String msg, Throwable e);
 
+    /**
+     * Check if any of the logged errors contain the given string.
+     * Default does nothing - 
+     * Override this method to use as needed.
+     * 
+     * @param msg
+     * @return
+     */
+    public boolean containsErrorMessage(String msg) {
+        return false;
+    }
+
     /**
      * Returns whether debug is enabled by the current logger
      * 

src/main/java/io/openliberty/tools/common/plugins/util/ServerFeatureUtil.java

@@ -459,7 +459,33 @@ private Set<String> parseIncludeNode(Set<String> origResult, File serverDirector
             return result;
         }
 
+        ArrayList<File> includeFiles = parseIncludeFileOrDirectory(includeFileName, includeFile);
+
+        for (File file : includeFiles) {
+            if (!updatedParsedXmls.contains(file)) {
+                String onConflict = node.getAttribute("onConflict");
+                Set<String> features = getServerXmlFeatures(null, serverDirectory, file, bootstrapProperties, updatedParsedXmls);
+                if (features != null && !features.isEmpty()) {
+                    info("Features were included for file "+ file.toString());
+                }
+                result = handleOnConflict(result, onConflict, features);
+            }
+        }
+        return result;
+    }
+
+    private ArrayList<File> parseIncludeFileOrDirectory(String includeFileName, File includeFile) {
+        // Earlier call to VariableUtility.resolveVariables() already converts all \ to /
+        boolean isLibertyDirectory = includeFileName.endsWith("/"); // Liberty uses this to determine if directory. 
         ArrayList<File> includeFiles = new ArrayList<File>();
+        if (includeFile.isFile() && isLibertyDirectory) {
+            error("Path specified a directory, but resource exists as a file (path=" + includeFileName + ")");
+            return includeFiles;
+        } else if (includeFile.isDirectory() && !isLibertyDirectory) {
+            error("Path specified a file, but resource exists as a directory (path=" + includeFileName + ")");
+            return includeFiles;
+        }
+
         if (includeFile.isDirectory()) {
             try (DirectoryStream<Path> dstream = Files.newDirectoryStream(includeFile.toPath(), "*.xml")) {
                 StreamSupport.stream(dstream.spliterator(), false)
@@ -477,19 +503,8 @@ private Set<String> parseIncludeNode(Set<String> origResult, File serverDirector
         } else {
             includeFiles.add(includeFile);
         }
-
-        for (File file : includeFiles) {
-            if (!updatedParsedXmls.contains(file)) {
-                String onConflict = node.getAttribute("onConflict");
-                Set<String> features = getServerXmlFeatures(null, serverDirectory, file, bootstrapProperties, updatedParsedXmls);
-                if (features != null && !features.isEmpty()) {
-                    info("Features were included for file "+ file.toString());
-                }
-                result = handleOnConflict(result, onConflict, features);
-            }
-        }
-        return result;
-    }
+        return includeFiles;
+    } 
 
     private static boolean isURL(String url) {
         try {

src/main/java/io/openliberty/tools/common/plugins/util/VariableUtility.java

@@ -28,10 +28,12 @@ public class VariableUtility {
     private static final String VARIABLE_NAME_PATTERN = "\\$\\{(.*?)\\}";
     private static final Pattern varNamePattern = Pattern.compile(VARIABLE_NAME_PATTERN);
 
-    /*
+    /**
      * Attempts to resolve all variables in the passed in nodeValue. Variable value/defaultValue can reference other variables.
      * This method is called recursively to resolve the variables. The variableChain collection keeps track of the variable references
      * in a resolution chain in order to prevent an infinite loop. The variableChain collection should be passed as null on the initial call.
+     * 
+     * NOTE: This method also replaces all back slashes with forward slashes
      */
     public static String resolveVariables(CommonLoggerI log, String nodeValue, Collection<String> variableChain, 
                                             Properties props, Properties defaultProps, Map<String, File> libDirPropFiles) {

src/test/java/io/openliberty/tools/common/plugins/util/BaseInstallFeatureUtilTest.java

@@ -41,6 +41,8 @@ public class BaseInstallFeatureUtilTest {
     public File installDir;
     public File buildDir;
     public String verify = "enforce";
+
+    private ArrayList<String> errorMessages = new ArrayList<String>();
 
     @Rule
     public TemporaryFolder temp = new TemporaryFolder();
@@ -87,13 +89,24 @@ public void info(String msg) {
         @Override
         public void error(String msg) {
             // not needed for tests
+            errorMessages.add(msg);
         }
 
         @Override
         public void error(String msg, Throwable e) {
             // not needed for tests
         }
 
+        @Override
+        public boolean containsErrorMessage(String msg) {
+            for (String error : errorMessages) {
+                if (error.contains(msg)) {
+                    return true;
+                }
+            }
+            return false;
+        }
+
         @Override
         public boolean isDebugEnabled() {
             return false;

src/test/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtilGetServerFeaturesTest.java

@@ -15,6 +15,7 @@
  */
 package io.openliberty.tools.common.plugins.util;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 import java.io.File;
 import java.io.IOException;
@@ -593,12 +594,13 @@ private void replaceIncludeLocation(String includeLocation) throws Exception {
     }
 
     /**
-     * Tests server.xml with include dir
+     * Tests server.xml with include dir (must end with trailing slash)
      * @throws Exception
      */
     @Test
     public void testIncludeDir() throws Exception {
-        replaceIncludeDir("includeDir");
+        // Note: Both the product code and test code end up converting Windows \ into /
+        replaceIncludeLocation("includeDir/"); 
         copy("includeDir");
 
         Set<String> expected = new HashSet<String>();
@@ -610,6 +612,23 @@ public void testIncludeDir() throws Exception {
         verifyServerFeatures(expected);
     }
 
+    /**
+     * Tests include directory without the trailing slash.
+     * Liberty treats this as a file, which conflicts with it being a dir, and throws an error.
+     * @throws Exception
+     */
+    @Test
+    public void testInvalidIncludeDir() throws Exception {
+        replaceIncludeLocation("includeDir"); 
+        copy("includeDir");
+
+        Set<String> expected = new HashSet<String>();
+        expected.add("orig");
+
+        verifyServerFeatures(expected);
+        assertTrue(util.containsErrorMessage("Path specified a file, but resource exists as a directory (path=includeDir)"));
+    }
+
     @Test
     public void testIncludeDirReplace() throws Exception {
         copyAsName("server_dir_replace.xml", "server.xml");
@@ -633,11 +652,6 @@ public void testIncludeDirIgnore() throws Exception {
 
         verifyServerFeatures(expected);
     }
-
-    private void replaceIncludeDir(String includeDirName) throws Exception {
-        File includeDir = new File(src, includeDirName);
-        replaceIncludeLocation(includeDir.getName());
-    }
 
     /**
      * Tests server.xml with user features

src/test/resources/serverConfig/liberty/wlp/usr/servers/defaultServer/server.xml

@@ -17,7 +17,7 @@
     <include location="${includeLocationNonDefault}/firstInclude.xml"/>    
     <webApplication location="${project.artifactId.four}.ear"/>
 
-    <include location="${server.config.dir}/includeDir"/>
+    <include location="${server.config.dir}/includeDir/"/>
     <webApplication location="${project.artifactId.five}.war"/>
     <webApplication location="${project.artifactId.six}.war"/>
 

src/test/resources/servers/server_dir_ignore.xml

@@ -3,5 +3,5 @@
     <featureManager>
         <feature>orig</feature>
     </featureManager>
-    <include location="includeDir" onConflict="IGNORE"/>
+    <include location="includeDir/" onConflict="IGNORE"/>
 </server>

src/test/resources/servers/server_dir_replace.xml

@@ -3,5 +3,5 @@
     <featureManager>
         <feature>orig</feature>
     </featureManager>
-    <include location="includeDir" onConflict="REPLACE"/>
+    <include location="includeDir/" onConflict="REPLACE"/>
 </server>