[GitHub] [hadoop] steveloughran commented on a change in pull request #2097: HADOOP-17088. Failed to load Xinclude files with relative path in cas…

[GitBox <gi...@apache.org>]

steveloughran commented on a change in pull request #2097:
URL: https://github.com/apache/hadoop/pull/2097#discussion_r449671131



##########
File path: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java
##########
@@ -3247,7 +3249,15 @@ private void handleInclude() throws XMLStreamException, IOException {
           File href = new File(confInclude);
           if (!href.isAbsolute()) {
             // Included resources are relative to the current resource
-            File baseFile = new File(name).getParentFile();
+            File baseFile;
+
+            try {
+              baseFile = new File(new URI(name));
+            } catch (IllegalArgumentException | URISyntaxException e) {
+              baseFile = new File(name);
+            }
+
+            baseFile = baseFile.getParentFile();
             href = new File(baseFile, href.getPath());

Review comment:
       I was worried here about what if baseFile = null at this point, but java.io.File is happy with that.
   

##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java
##########
@@ -1062,6 +1062,38 @@ public void testRelativeIncludes() throws Exception {
     new File(new File(relConfig).getParent()).delete();
   }
 
+  @Test
+  public void testRelativeIncludesWithLoadingViaUri() throws Exception {
+    tearDown();
+    File configFile = new File("./tmp/test-config.xml");
+    File configFile2 = new File("./tmp/test-config2.xml");
+
+    new File(configFile.getParent()).mkdirs();
+    out = new BufferedWriter(new FileWriter(configFile2));
+    startConfig();
+    appendProperty("a", "b");
+    endConfig();
+
+    out = new BufferedWriter(new FileWriter(configFile));
+    startConfig();
+    // Add the relative path instead of the absolute one.
+    startInclude(configFile2.getName());
+    endInclude();
+    appendProperty("c", "d");
+    endConfig();
+
+    // verify that the includes file contains all properties
+    Path fileResource = new Path(configFile.toURI());
+    conf.addResource(fileResource);
+    assertEquals(conf.get("a"), "b");

Review comment:
       params are the wrong way round. not your fault, but there's no reason to replicate the existing issue.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org