fix: unescape tabs before writing to pom.xml (#1190)

#1184 When encoding tokens to pom.xml, tabs are escaped and this is not what we want. In this PR, before writing to pom.xml, we replace all escaped tabs with unescaped characters.
google · Aug 21, 2024 · dd4eef1 · dd4eef1
1 parent 61979fe
commit dd4eef1
Show file tree

Hide file tree

Showing 4 changed files with 206 additions and 38 deletions.
diff --git a/internal/resolution/manifest/__snapshots__/maven_test.snap b/internal/resolution/manifest/__snapshots__/maven_test.snap
@@ -1,4 +1,148 @@
 
+[TestMavenReadWrite - 1]
+<?xml version="1.0" encoding="UTF-8"?>
+
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+
+  <groupId>com.mycompany.app</groupId>
+  <artifactId>my-app</artifactId>
+  <version>1.0</version>
+
+  <name>my-app</name>
+  <!-- FIXME change it to the project's website -->
+  <url>http://www.example.com</url>
+
+  <parent>
+    <groupId>org.parent</groupId>
+    <artifactId>parent-pom</artifactId>
+    <version>1.1.1</version>
+    <relativePath>../parent/pom.xml</relativePath>
+  </parent>
+
+  <properties>
+    <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+    <maven.compiler.source>1.7</maven.compiler.source>
+    <maven.compiler.target>1.7</maven.compiler.target>
+    <junit.version>4.12</junit.version>
+      <zeppelin.daemon.package.base>
+        ../bin
+      </zeppelin.daemon.package.base>
+  </properties>
+
+  <dependencies>
+    <dependency>
+      <groupId>junit</groupId>
+      <artifactId>junit</artifactId>
+      <version>${junit.version}</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.example</groupId>
+      <artifactId>abc</artifactId>
+      <version>1.0.1</version>
+    </dependency>
+    <dependency>
+      <groupId>org.example</groupId>
+      <artifactId>no-version</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.example</groupId>
+      <artifactId>exclusions</artifactId>
+      <version>1.0.0</version>
+      <exclusions>
+        <exclusion>
+          <groupId>org.exclude</groupId>
+          <artifactId>exclude</artifactId>
+        </exclusion>
+      </exclusions>
+    </dependency>
+  </dependencies>
+
+  <dependencyManagement>
+    <dependencies>
+      <dependency>
+        <groupId>org.example</groupId>
+        <artifactId>xyz</artifactId>
+        <version>2.0.0</version>
+      </dependency>
+      <dependency>
+        <groupId>org.example</groupId>
+        <artifactId>no-version</artifactId>
+        <version>2.0.0</version>
+      </dependency>
+      <dependency>
+        <groupId>org.import</groupId>
+        <artifactId>import</artifactId>
+        <version>1.0.0</version>
+        <type>pom</type>
+        <scope>import</scope>
+      </dependency>
+    </dependencies>
+  </dependencyManagement>
+
+  <profiles>
+    <profile>
+      <id>profile-one</id>
+      <activation>
+        <activeByDefault>true</activeByDefault>
+      </activation>
+      <properties>
+        <def.version>2.3.4</def.version>
+      </properties>
+      <dependencies>
+        <dependency>
+          <groupId>org.profile</groupId>
+          <artifactId>abc</artifactId>
+          <version>1.2.3</version>
+        </dependency>
+        <dependency>
+          <groupId>org.profile</groupId>
+          <artifactId>def</artifactId>
+          <version>${def.version}</version>
+        </dependency>
+      </dependencies>
+    </profile>
+    <profile>
+      <id>profile-two</id>
+      <dependencyManagement>
+        <dependencies>
+          <dependency>
+            <groupId>org.import</groupId>
+            <artifactId>xyz</artifactId>
+            <version>6.6.6</version>
+            <scope>import</scope>
+            <type>pom</type>
+          </dependency>
+        </dependencies>
+      </dependencyManagement>
+    </profile>
+  </profiles>
+
+  <build>
+    <pluginManagement>
+      <plugins>
+        <plugin>
+          <groupId>org.plugin</groupId>
+          <artifactId>plugin</artifactId>
+          <version>1.0.0</version>
+          <dependencies>
+            <dependency>
+              <groupId>org.dep</groupId>
+              <artifactId>plugin-dep</artifactId>
+              <version>2.3.3</version>
+            </dependency>
+          </dependencies>
+        </plugin>
+      </plugins>
+    </pluginManagement>
+  </build>
+
+</project>
+
+---
+
 [TestMavenWrite - 1]
 <?xml version="1.0" encoding="UTF-8"?>
 
@@ -26,7 +170,9 @@
     <maven.compiler.source>1.7</maven.compiler.source>
     <maven.compiler.target>1.7</maven.compiler.target>
     <junit.version>4.13.2</junit.version>
-    <zeppelin.daemon.package.base>../bin</zeppelin.daemon.package.base>
+&#x9;  <zeppelin.daemon.package.base>
+&#x9;    ../bin
+&#x9;  </zeppelin.daemon.package.base>
   </properties>
 
   <dependencies>

diff --git a/internal/resolution/manifest/fixtures/maven/my-app/pom.xml b/internal/resolution/manifest/fixtures/maven/my-app/pom.xml
@@ -24,9 +24,9 @@
     <maven.compiler.source>1.7</maven.compiler.source>
     <maven.compiler.target>1.7</maven.compiler.target>
     <junit.version>4.12</junit.version>
-    <zeppelin.daemon.package.base>
+	  <zeppelin.daemon.package.base>
 	    ../bin
-    </zeppelin.daemon.package.base>
+	  </zeppelin.daemon.package.base>
   </properties>
 
   <dependencies>

diff --git a/internal/resolution/manifest/maven.go b/internal/resolution/manifest/maven.go
@@ -330,6 +330,7 @@ func (MavenManifestIO) Write(df lockfile.DepFile, w io.Writer, patch ManifestPat
 	if err != nil {
 		return err
 	}
+
 	for path, patches := range allPatches {
 		if path == "" {
 			// Base pom.xml is going to be written later.
@@ -346,22 +347,34 @@ func (MavenManifestIO) Write(df lockfile.DepFile, w io.Writer, patch ManifestPat
 		}
 		depFile.Close() // Make sure the file is closed before we start writing to it.
 
-		var out bytes.Buffer
-		if err := write(in, &out, patches); err != nil {
+		out := new(bytes.Buffer)
+		if err := write(in.String(), out, patches); err != nil {
 			return err
 		}
 		//nolint:gosec
-		if err := os.WriteFile(path, out.Bytes(), 0644); err != nil {
+		if err := os.WriteFile(path, unescape(out.String()), 0644); err != nil {
 			return err
 		}
 	}
 
-	buf := new(bytes.Buffer)
-	if _, err := buf.ReadFrom(df); err != nil {
+	in := new(bytes.Buffer)
+	if _, err := in.ReadFrom(df); err != nil {
 		return fmt.Errorf("failed to read from DepFile: %w", err)
 	}
+	out := new(bytes.Buffer)
+	if err := write(in.String(), out, allPatches[""]); err != nil {
+		return fmt.Errorf("failed to write pom.xml: %w", err)
+	}
+	_, err = w.Write(unescape(out.String()))
+
+	return err
+}
+
+// Unescape replaces escaped tabs with unescaped characters.
+func unescape(out string) []byte {
+	out = strings.ReplaceAll(out, "&#x9;", "\t")
 
-	return write(buf, w, allPatches[""])
+	return []byte(out)
 }
 
 type MavenPatches struct {
@@ -629,8 +642,8 @@ func compareDependency(d1, d2 dependency) int {
 	return cmp.Compare(d1.Version, d2.Version)
 }
 
-func write(buf *bytes.Buffer, w io.Writer, patches MavenPatches) error {
-	dec := xml.NewDecoder(bytes.NewReader(buf.Bytes()))
+func write(raw string, w io.Writer, patches MavenPatches) error {
+	dec := xml.NewDecoder(bytes.NewReader([]byte(raw)))
 	enc := xml.NewEncoder(w)
 
 	for {
@@ -657,7 +670,7 @@ func write(buf *bytes.Buffer, w io.Writer, patches MavenPatches) error {
 				// Thus this would cause a big diff when we try to encode the start element of project.
 
 				// We first capture the raw start element string and write it.
-				projectStart := projectStartElement(buf.String())
+				projectStart := projectStartElement(raw)
 				if projectStart == "" {
 					return errors.New("unable to get start element of project")
 				}
@@ -741,7 +754,7 @@ func writeProject(w io.Writer, enc *xml.Encoder, raw, prefix, id string, patches
 						req = k.NewRequire
 					}
 				}
-				if err := writeString(enc, "<parent>"+rawParent.InnerXML+"</parent>", []string{"parent"}, map[string]string{"version": req}); err != nil {
+				if err := writeString(enc, "<parent>"+rawParent.InnerXML+"</parent>", map[string]string{"version": req}); err != nil {
 					return fmt.Errorf("updating parent: %w", err)
 				}
 
@@ -754,7 +767,7 @@ func writeProject(w io.Writer, enc *xml.Encoder, raw, prefix, id string, patches
 				if err := dec.DecodeElement(&rawProperties, &tt); err != nil {
 					return err
 				}
-				if err := writeString(enc, "<properties>"+rawProperties.InnerXML+"</properties>", []string{"properties", "property"}, properties[mavenOrigin(prefix, id)]); err != nil {
+				if err := writeString(enc, "<properties>"+rawProperties.InnerXML+"</properties>", properties[mavenOrigin(prefix, id)]); err != nil {
 					return fmt.Errorf("updating properties: %w", err)
 				}
 
@@ -906,7 +919,7 @@ func writeDependency(w io.Writer, enc *xml.Encoder, raw string, patches map[Mave
 				}
 				// xml.EncodeElement writes all empty elements and may not follow the existing format.
 				// Passing the innerXML can help to keep the original format.
-				if err := writeString(enc, "<dependency>"+rawDep.InnerXML+"</dependency>", []string{"dependency", "exclusion", "exclusions"}, map[string]string{"version": req}); err != nil {
+				if err := writeString(enc, "<dependency>"+rawDep.InnerXML+"</dependency>", map[string]string{"version": req}); err != nil {
 					return fmt.Errorf("updating dependency: %w", err)
 				}
 
@@ -923,10 +936,7 @@ func writeDependency(w io.Writer, enc *xml.Encoder, raw string, patches map[Mave
 }
 
 // writeString writes XML string specified by raw with replacements specified in values.
-// skipTags specifies the tags we skip for writing (usually higher level tags).
-// White space is trimmed during writing to prevent the text being escaped.
-// TODO: investigate if we can rely on the nesting level to decide trimming or not.
-func writeString(enc *xml.Encoder, raw string, skipTags []string, values map[string]string) error {
+func writeString(enc *xml.Encoder, raw string, values map[string]string) error {
 	dec := xml.NewDecoder(bytes.NewReader([]byte(raw)))
 	for {
 		token, err := dec.Token()
@@ -936,20 +946,18 @@ func writeString(enc *xml.Encoder, raw string, skipTags []string, values map[str
 		if err != nil {
 			return err
 		}
-		if tt, ok := token.(xml.StartElement); ok && !slices.Contains(skipTags, tt.Name.Local) {
-			var str string
-			if err := dec.DecodeElement(&str, &tt); err != nil {
-				return err
-			}
+		if tt, ok := token.(xml.StartElement); ok {
 			if value, ok2 := values[tt.Name.Local]; ok2 {
-				str = value
-			}
-			// Trim the white space before encoding the string, otherwise the text is escaped.
-			if err := enc.EncodeElement(strings.TrimSpace(str), tt); err != nil {
-				return err
-			}
+				var str string
+				if err := dec.DecodeElement(&str, &tt); err != nil {
+					return err
+				}
+				if err := enc.EncodeElement(value, tt); err != nil {
+					return err
+				}
 
-			continue
+				continue
+			}
 		}
 		if err := enc.EncodeToken(token); err != nil {
 			return err

diff --git a/internal/resolution/manifest/maven_test.go b/internal/resolution/manifest/maven_test.go
@@ -53,7 +53,7 @@ func mavenReqKey(t *testing.T, name, artifactType, classifier string) Requiremen
 	})
 }
 
-func TestMavenRead(t *testing.T) {
+func TestMavenReadWrite(t *testing.T) {
 	t.Parallel()
 
 	srv := testutility.NewMockHTTPServer(t)
@@ -411,6 +411,20 @@ func TestMavenRead(t *testing.T) {
 	if diff := cmp.Diff(got, want); diff != "" {
 		t.Errorf("Maven manifest mismatch: %s", diff)
 	}
+
+	// Re-open the file for writing.
+	df, err = lockfile.OpenLocalDepFile(filepath.Join(dir, "fixtures", "maven", "my-app", "pom.xml"))
+	if err != nil {
+		t.Fatalf("failed to open file: %v", err)
+	}
+	defer df.Close()
+
+	out := new(bytes.Buffer)
+	// There are no patches since we are only testing tabs are not escaped.
+	if err := mavenIO.Write(df, out, ManifestPatch{Manifest: &want}); err != nil {
+		t.Fatalf("failed to write Maven pom.xml: %v", err)
+	}
+	testutility.NewSnapshot().WithCRLFReplacement().MatchText(t, out.String())
 }
 
 func TestMavenWrite(t *testing.T) {
@@ -426,8 +440,8 @@ func TestMavenWrite(t *testing.T) {
 	}
 	defer df.Close()
 
-	buf := new(bytes.Buffer)
-	if _, err := buf.ReadFrom(df); err != nil {
+	in := new(bytes.Buffer)
+	if _, err := in.ReadFrom(df); err != nil {
 		t.Fatalf("failed to read from DepFile: %v", err)
 	}
 
@@ -519,7 +533,7 @@ func TestMavenWrite(t *testing.T) {
 	}
 
 	out := new(bytes.Buffer)
-	if err := write(buf, out, patches); err != nil {
+	if err := write(in.String(), out, patches); err != nil {
 		t.Fatalf("unable to update Maven pom.xml: %v", err)
 	}
 	testutility.NewSnapshot().WithCRLFReplacement().MatchText(t, out.String())
@@ -538,8 +552,8 @@ func TestMavenWriteDM(t *testing.T) {
 	}
 	defer df.Close()
 
-	buf := new(bytes.Buffer)
-	if _, err := buf.ReadFrom(df); err != nil {
+	in := new(bytes.Buffer)
+	if _, err := in.ReadFrom(df); err != nil {
 		t.Fatalf("failed to read from DepFile: %v", err)
 	}
 
@@ -587,7 +601,7 @@ func TestMavenWriteDM(t *testing.T) {
 	}
 
 	out := new(bytes.Buffer)
-	if err := write(buf, out, patches); err != nil {
+	if err := write(in.String(), out, patches); err != nil {
 		t.Fatalf("unable to update Maven pom.xml: %v", err)
 	}
 	testutility.NewSnapshot().WithCRLFReplacement().MatchText(t, out.String())