From d5b02e9be4fb1a775ab749a0d0f7e4f8920f1509 Mon Sep 17 00:00:00 2001 From: Evie Lau <689163+evie-lau@users.noreply.github.com> Date: Fri, 27 Oct 2023 10:19:57 -0500 Subject: [PATCH] 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 --- .../plugins/config/ServerConfigDocument.java | 37 ++++++++++------- .../plugins/util/InstallFeatureUtil.java | 12 ++++++ .../plugins/util/ServerFeatureUtil.java | 41 +++++++++++++------ .../common/plugins/util/VariableUtility.java | 4 +- .../util/BaseInstallFeatureUtilTest.java | 13 ++++++ ...stallFeatureUtilGetServerFeaturesTest.java | 28 +++++++++---- .../wlp/usr/servers/defaultServer/server.xml | 2 +- .../resources/servers/server_dir_ignore.xml | 2 +- .../resources/servers/server_dir_replace.xml | 2 +- 9 files changed, 103 insertions(+), 38 deletions(-) diff --git a/src/main/java/io/openliberty/tools/common/plugins/config/ServerConfigDocument.java b/src/main/java/io/openliberty/tools/common/plugins/config/ServerConfigDocument.java index 2dbb50e6..b4b65cdb 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/config/ServerConfigDocument.java +++ b/src/main/java/io/openliberty/tools/common/plugins/config/ServerConfigDocument.java @@ -450,11 +450,8 @@ private static ArrayList 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 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 getIncludeDocs(String loc) throws IOException /** * Parses file or directory for all xml documents, and adds to ArrayList - * @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 docs) throws FileNotFoundException, IOException, SAXException { + private static void parseDocumentFromFileOrDirectory(File f, String locationString, ArrayList 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); } } diff --git a/src/main/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtil.java b/src/main/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtil.java index 49dd01bd..aabacc01 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtil.java +++ b/src/main/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtil.java @@ -284,6 +284,18 @@ private Set 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 * diff --git a/src/main/java/io/openliberty/tools/common/plugins/util/ServerFeatureUtil.java b/src/main/java/io/openliberty/tools/common/plugins/util/ServerFeatureUtil.java index f59e9d4d..8ca9c2ea 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/util/ServerFeatureUtil.java +++ b/src/main/java/io/openliberty/tools/common/plugins/util/ServerFeatureUtil.java @@ -459,7 +459,33 @@ private Set parseIncludeNode(Set origResult, File serverDirector return result; } + ArrayList includeFiles = parseIncludeFileOrDirectory(includeFileName, includeFile); + + for (File file : includeFiles) { + if (!updatedParsedXmls.contains(file)) { + String onConflict = node.getAttribute("onConflict"); + Set 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 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 includeFiles = new ArrayList(); + 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 dstream = Files.newDirectoryStream(includeFile.toPath(), "*.xml")) { StreamSupport.stream(dstream.spliterator(), false) @@ -477,19 +503,8 @@ private Set parseIncludeNode(Set origResult, File serverDirector } else { includeFiles.add(includeFile); } - - for (File file : includeFiles) { - if (!updatedParsedXmls.contains(file)) { - String onConflict = node.getAttribute("onConflict"); - Set 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 { diff --git a/src/main/java/io/openliberty/tools/common/plugins/util/VariableUtility.java b/src/main/java/io/openliberty/tools/common/plugins/util/VariableUtility.java index e90f85b4..46d5471f 100644 --- a/src/main/java/io/openliberty/tools/common/plugins/util/VariableUtility.java +++ b/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 variableChain, Properties props, Properties defaultProps, Map libDirPropFiles) { diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/BaseInstallFeatureUtilTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/BaseInstallFeatureUtilTest.java index ff13eba1..3460fc4f 100644 --- a/src/test/java/io/openliberty/tools/common/plugins/util/BaseInstallFeatureUtilTest.java +++ b/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 errorMessages = new ArrayList(); @Rule public TemporaryFolder temp = new TemporaryFolder(); @@ -87,6 +89,7 @@ public void info(String msg) { @Override public void error(String msg) { // not needed for tests + errorMessages.add(msg); } @Override @@ -94,6 +97,16 @@ 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; diff --git a/src/test/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtilGetServerFeaturesTest.java b/src/test/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtilGetServerFeaturesTest.java index 7c0d75aa..250ab946 100644 --- a/src/test/java/io/openliberty/tools/common/plugins/util/InstallFeatureUtilGetServerFeaturesTest.java +++ b/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 expected = new HashSet(); @@ -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 expected = new HashSet(); + 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 diff --git a/src/test/resources/serverConfig/liberty/wlp/usr/servers/defaultServer/server.xml b/src/test/resources/serverConfig/liberty/wlp/usr/servers/defaultServer/server.xml index 95590086..bc801f17 100644 --- a/src/test/resources/serverConfig/liberty/wlp/usr/servers/defaultServer/server.xml +++ b/src/test/resources/serverConfig/liberty/wlp/usr/servers/defaultServer/server.xml @@ -17,7 +17,7 @@ - + diff --git a/src/test/resources/servers/server_dir_ignore.xml b/src/test/resources/servers/server_dir_ignore.xml index c9f70e38..658eb70a 100644 --- a/src/test/resources/servers/server_dir_ignore.xml +++ b/src/test/resources/servers/server_dir_ignore.xml @@ -3,5 +3,5 @@ orig - + \ No newline at end of file diff --git a/src/test/resources/servers/server_dir_replace.xml b/src/test/resources/servers/server_dir_replace.xml index 26028728..c9afc4d5 100644 --- a/src/test/resources/servers/server_dir_replace.xml +++ b/src/test/resources/servers/server_dir_replace.xml @@ -3,5 +3,5 @@ orig - + \ No newline at end of file