Stop using regex to update dependencies in builder.toml, package.toml…

…, and buildpack.toml

This includes:
- metadata dependencies
- composite buildpack.toml order groups
- package.toml files
- builder.toml files

Instead of using complicated, fragile regular expressions the code unmarshals the file from TOML to a map, traverses the map, locates the items to update, if found updates them, and then writes out the map as TOML. Using maps is a pain here, but it's done because unmarshaling to structs would require a lot of different, complicated structs that don't already exist. This would be a lot of effort because we only care about a very small subset of these files, creating structs to map out a bunch of stuff we don't care about.

Great care was put into ensuring that we are checking for keys to exist and for type assertions to be true. This should return sensible errors or skip bad data where possible.

The one downside of this approach that should be noted is that the TOML generated when we marshal the map back out to a file is controlled 100% by the TOML library. Thus the order for these files will likely change after the first time a dependency is updated. Fortunately, the order defined by the TOML library is static, so it won't change from update-to-update. There are just no options to control how it writes the TOML, so it's not possible to match our existing format. This includes erasing comments. The code specifically handles leading comments, which are often license headers, to ensure that they are preserved. Other comments will be removed though.

On the plus side, this makes the update process way more robust because it also does not care about the incoming TOML format either. Any valid TOML format will work, which was not the case with the previous regular expression based update approach. That required a very specific TOML format. In addition, the TOML format output by the library is less compact & this should reduce the frequency of merge conflicts when updating dependencies of buildpacks.

Signed-off-by: Daniel Mikusa <dmikusa@vmware.com>

carton/buildpack_dependency.go

@@ -17,13 +17,15 @@
 package carton
 
 import (
+	"bytes"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"regexp"
 
 	"github.com/paketo-buildpacks/libpak/bard"
 	"github.com/paketo-buildpacks/libpak/internal"
+	"github.com/pelletier/go-toml"
 )
 
 const (
@@ -55,26 +57,94 @@ func (b BuildpackDependency) Update(options ...Option) {
 	logger.Headerf("URI:     %s", b.URI)
 	logger.Headerf("SHA256:  %s", b.SHA256)
 
+	versionExp, err := regexp.Compile(b.VersionPattern)
+	if err != nil {
+		config.exitHandler.Error(fmt.Errorf("unable to compile regex %s\n%w", b.VersionPattern, err))
+		return
+	}
+
 	c, err := ioutil.ReadFile(b.BuildpackPath)
 	if err != nil {
 		config.exitHandler.Error(fmt.Errorf("unable to read %s\n%w", b.BuildpackPath, err))
 		return
 	}
 
-	s := fmt.Sprintf(BuildpackDependencyPattern, b.ID, b.VersionPattern)
-	r, err := regexp.Compile(s)
-	if err != nil {
-		config.exitHandler.Error(fmt.Errorf("unable to compile regex %s\n%w", s, err))
+	// save any leading comments, this is to preserve license headers
+	// inline comments will be lost
+	comments := []byte{}
+	for _, line := range bytes.SplitAfter(c, []byte("\n")) {
+		if bytes.HasPrefix(line, []byte("#")) || len(bytes.TrimSpace(line)) == 0 {
+			comments = append(comments, line...)
+		} else {
+			break // stop on first comment
+		}
+	}
+
+	md := make(map[string]interface{})
+	if err := toml.Unmarshal(c, &md); err != nil {
+		config.exitHandler.Error(fmt.Errorf("unable to decode md%s\n%w", b.BuildpackPath, err))
+		return
+	}
+
+	metadataUnwrapped, found := md["metadata"]
+	if !found {
+		config.exitHandler.Error(fmt.Errorf("unable to find metadata block"))
+		return
+	}
+
+	metadata, ok := metadataUnwrapped.(map[string]interface{})
+	if !ok {
+		config.exitHandler.Error(fmt.Errorf("unable to cast metadata"))
+		return
+	}
+
+	dependenciesUnwrapped, found := metadata["dependencies"]
+	if !found {
+		config.exitHandler.Error(fmt.Errorf("unable to find dependencies block"))
+		return
+	}
+
+	dependencies, ok := dependenciesUnwrapped.([]map[string]interface{})
+	if !ok {
+		config.exitHandler.Error(fmt.Errorf("unable to cast dependencies"))
 		return
 	}
 
-	if !r.Match(c) {
-		config.exitHandler.Error(fmt.Errorf("unable to match '%s' '%s'", b.ID, b.VersionPattern))
+	for _, dep := range dependencies {
+		depIdUnwrapped, found := dep["id"]
+		if !found {
+			continue
+		}
+		depId, ok := depIdUnwrapped.(string)
+		if !ok {
+			continue
+		}
+
+		if depId == b.ID {
+			depVersionUnwrapped, found := dep["version"]
+			if !found {
+				continue
+			}
+
+			depVersion, ok := depVersionUnwrapped.(string)
+			if !ok {
+				continue
+			}
+			if versionExp.MatchString(depVersion) {
+				dep["version"] = b.Version
+				dep["uri"] = b.URI
+				dep["sha256"] = b.SHA256
+			}
+		}
+	}
+
+	c, err = toml.Marshal(md)
+	if err != nil {
+		config.exitHandler.Error(fmt.Errorf("unable to encode md %s\n%w", b.BuildpackPath, err))
 		return
 	}
 
-	s = fmt.Sprintf(BuildpackDependencySubstitution, b.Version, b.URI, b.SHA256)
-	c = r.ReplaceAll(c, []byte(s))
+	c = append(comments, c...)
 
 	if err := ioutil.WriteFile(b.BuildpackPath, c, 0644); err != nil {
 		config.exitHandler.Error(fmt.Errorf("unable to write %s\n%w", b.BuildpackPath, err))

carton/buildpack_dependency_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/stretchr/testify/mock"
 
 	"github.com/paketo-buildpacks/libpak/carton"
+	"github.com/paketo-buildpacks/libpak/internal"
 )
 
 func testBuildpackDependency(t *testing.T, context spec.G, it spec.S) {
@@ -54,7 +55,14 @@ func testBuildpackDependency(t *testing.T, context spec.G, it spec.S) {
 	})
 
 	it("updates dependency", func() {
-		Expect(ioutil.WriteFile(path, []byte(`id      = "test-id"
+		Expect(ioutil.WriteFile(path, []byte(`api = "0.6"
+[buildpack]
+id = "some-buildpack"
+name = "Some Buildpack"
+version = "1.2.3"
+
+[[metadata.dependencies]]
+id      = "test-id"
 name    = "Test Name"
 version = "test-version-1"
 uri     = "test-uri-1"
@@ -73,17 +81,34 @@ stacks  = [ "test-stack" ]
 
 		d.Update(carton.WithExitHandler(exitHandler))
 
-		Expect(ioutil.ReadFile(path)).To(Equal([]byte(`id      = "test-id"
+		Expect(ioutil.ReadFile(path)).To(internal.MatchTOML(`api = "0.6"
+[buildpack]
+id = "some-buildpack"
+name = "Some Buildpack"
+version = "1.2.3"
+
+[[metadata.dependencies]]id      = "test-id"
 name    = "Test Name"
 version = "test-version-2"
 uri     = "test-uri-2"
 sha256  = "test-sha256-2"
 stacks  = [ "test-stack" ]
-`)))
+`))
 	})
 
 	it("updates indented dependency", func() {
-		Expect(ioutil.WriteFile(path, []byte(`  id      = "test-id"
+		Expect(ioutil.WriteFile(path, []byte(`# it should preserve
+#   these comments
+#      exactly
+
+api = "0.6"
+[buildpack]
+id = "some-buildpack"
+name = "Some Buildpack"
+version = "1.2.3"
+
+[[metadata.dependencies]]
+  id      = "test-id"
   name    = "Test Name"
   version = "test-version-1"
   uri     = "test-uri-1"
@@ -102,13 +127,26 @@ stacks  = [ "test-stack" ]
 
 		d.Update(carton.WithExitHandler(exitHandler))
 
-		Expect(ioutil.ReadFile(path)).To(Equal([]byte(`  id      = "test-id"
+		body, err := ioutil.ReadFile(path)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(string(body)).To(HavePrefix(`# it should preserve
+#   these comments
+#      exactly
+
+api = "0.6"`))
+		Expect(body).To(internal.MatchTOML(`api = "0.6"
+[buildpack]
+id = "some-buildpack"
+name = "Some Buildpack"
+version = "1.2.3"
+
+[[metadata.dependencies]]
+  id      = "test-id"
   name    = "Test Name"
   version = "test-version-2"
   uri     = "test-uri-2"
   sha256  = "test-sha256-2"
   stacks  = [ "test-stack" ]
-`)))
+`))
 	})
-
 }

carton/package.go

@@ -188,7 +188,7 @@ func (p Package) Create(options ...Option) {
 	}
 
 	var files []string
-	for d, _ := range entries {
+	for d := range entries {
 		files = append(files, d)
 	}
 	sort.Strings(files)