fix(internal/aliasfix): handle import paths correctly (#10097)

egonelbre · web-flow · commit fafaf0d0a293 · 2024-05-03T19:54:17.000Z
The last component in import path of the old genproto path does not correspond to the import path -- usually.

The import paths usually look like `google.golang.org/genproto/googleapis/analytics/admin/v1alpha`, however the imported package name is `admin`. This uses a heuristic to detect whether the last path component is a version,

There are some packages that do not fit that description though, e.g. `google.golang.org/genproto/googleapis/devtools/containeranalysis/v1beta1/grafeas`.
diff --git a/internal/aliasfix/aliasfix.go b/internal/aliasfix/aliasfix.go
@@ -26,6 +26,7 @@ import (
 	"io"
 	"io/fs"
 	"os"
+	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -46,12 +47,12 @@ func ProcessPath(path string) error {
 	}
 	if dir.IsDir() {
 		err := filepath.WalkDir(path, func(path string, d fs.DirEntry, err error) error {
-			if err == nil && !d.IsDir() && strings.HasSuffix(d.Name(), ".go") {
-				err = processFile(path, nil)
-			}
-			if err != nil {
+			if err != nil || d.IsDir() {
 				return err
 			}
+			if strings.HasSuffix(d.Name(), ".go") {
+				return processFile(path, nil)
+			}
 			return nil
 		})
 		if err != nil {
@@ -73,6 +74,7 @@ func processFile(name string, w io.Writer) (err error) {
 	if err != nil {
 		return err
 	}
+
 	var modified bool
 	for _, imp := range f.Imports {
 		var importPath string
@@ -81,8 +83,8 @@ func processFile(name string, w io.Writer) (err error) {
 			return err
 		}
 		if pkg, ok := GenprotoPkgMigration[importPath]; ok && pkg.Status == StatusMigrated {
-			oldNamespace := importPath[strings.LastIndex(importPath, "/")+1:]
-			newNamespace := pkg.ImportPath[strings.LastIndex(pkg.ImportPath, "/")+1:]
+			oldNamespace := genprotoNamespace(importPath)
+			newNamespace := path.Base(pkg.ImportPath)
 			if imp.Name == nil && oldNamespace != newNamespace {
 				// use old namespace for fewer diffs
 				imp.Name = ast.NewIdent(oldNamespace)
@@ -99,26 +101,6 @@ func processFile(name string, w io.Writer) (err error) {
 		return nil
 	}
 
-	if w == nil {
-		backup := name + ".bak"
-		if err = os.Rename(name, backup); err != nil {
-			return err
-		}
-		defer func() {
-			if err != nil {
-				os.Rename(backup, name)
-			} else {
-				os.Remove(backup)
-			}
-		}()
-		var file *os.File
-		file, err = os.Create(name)
-		if err != nil {
-			return err
-		}
-		defer file.Close()
-		w = file
-	}
 	var buf bytes.Buffer
 	if err := format.Node(&buf, fset, f); err != nil {
 		return err
@@ -127,9 +109,32 @@ func processFile(name string, w io.Writer) (err error) {
 	if err != nil {
 		return err
 	}
-	if _, err := w.Write(b); err != nil {
+
+	if w != nil {
+		_, err := w.Write(b)
 		return err
 	}
 
-	return nil
+	backup := name + ".bak"
+	if err = os.Rename(name, backup); err != nil {
+		return err
+	}
+	defer func() {
+		if err != nil {
+			os.Rename(backup, name)
+		} else {
+			os.Remove(backup)
+		}
+	}()
+
+	return os.WriteFile(name, b, 0644)
+}
+
+func genprotoNamespace(importPath string) string {
+	suffix := path.Base(importPath)
+	// if it looks like a version, then use the second from last component.
+	if len(suffix) >= 2 && suffix[0] == 'v' && '0' <= suffix[1] && suffix[1] <= '1' {
+		return path.Base(path.Dir(importPath))
+	}
+	return suffix
 }
diff --git a/internal/aliasfix/aliasfix_test.go b/internal/aliasfix/aliasfix_test.go
@@ -74,10 +74,14 @@ func TestGolden(t *testing.T) {
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			GenprotoPkgMigration["example.com/old/foo"] = Pkg{
+			GenprotoPkgMigration["example.com/old/foo/v1"] = Pkg{
 				ImportPath: "example.com/new/foopb",
 				Status:     tc.status,
 			}
+			GenprotoPkgMigration["example.com/old/bar/v1/bar"] = Pkg{
+				ImportPath: "example.com/new/barpb",
+				Status:     tc.status,
+			}
 			var w bytes.Buffer
 			if updateGoldens {
 				if err := processFile(filepath.Join("testdata", tc.fileName), nil); err != nil {
diff --git a/internal/aliasfix/testdata/golden/input2 b/internal/aliasfix/testdata/golden/input2
@@ -3,7 +3,9 @@ package golden
 import (
 	"net"
 
+	bar "example.com/new/barpb"
 	foo "example.com/new/foopb"
 )
 
 func Bar1(foo foo.Baz, addr net.Addr) {}
+func Bar2(foo bar.Baz, addr net.Addr) {}
diff --git a/internal/aliasfix/testdata/input1 b/internal/aliasfix/testdata/input1
@@ -1,5 +1,5 @@
 package golden
 
-import "example.com/old/foo"
+import "example.com/old/foo/v1"
 
 func Bar1(foo foo.Baz) {}
diff --git a/internal/aliasfix/testdata/input2 b/internal/aliasfix/testdata/input2
@@ -3,7 +3,9 @@ package golden
 import (
 	"net"
 
-	"example.com/old/foo"
+	"example.com/old/foo/v1"
+	"example.com/old/bar/v1/bar"
 )
 
 func Bar1(foo foo.Baz, addr net.Addr) {}
+func Bar2(foo bar.Baz, addr net.Addr) {}
diff --git a/internal/aliasfix/testdata/input4 b/internal/aliasfix/testdata/input4
@@ -1,5 +1,5 @@
 package golden
 
-import foopb "example.com/old/foo"
+import foopb "example.com/old/foo/v1"
 
 func Bar4(baz foopb.Baz) {}
diff --git a/internal/aliasfix/testdata/input5 b/internal/aliasfix/testdata/input5
@@ -3,7 +3,7 @@ package golden
 import (
 	"net"
 
-	blah "example.com/old/foo"
+	blah "example.com/old/foo/v1"
 )
 
 func Bar2(baz blah.Baz, addr net.Addr) {}
diff --git a/internal/aliasfix/testdata/input6 b/internal/aliasfix/testdata/input6
@@ -3,7 +3,7 @@ package golden
 import (
 	"net"
 
-	blah "example.com/old/foo"
+	blah "example.com/old/foo/v1"
 )
 
 func Bar2(baz blah.Baz, addr net.Addr) {}

Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,9 @@ package golden`
`3`	`3`	`import (`
`4`	`4`	`"net"`
`5`	`5`
	`6`	`+ bar "example.com/new/barpb"`
`6`	`7`	`foo "example.com/new/foopb"`
`7`	`8`	`)`
`8`	`9`
`9`	`10`	`func Bar1(foo foo.Baz, addr net.Addr) {}`
	`11`	`+func Bar2(foo bar.Baz, addr net.Addr) {}`
Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,7 @@ package golden`
`3`	`3`	`import (`
`4`	`4`	`"net"`
`5`	`5`
`6`		`- blah "example.com/old/foo"`
	`6`	`+ blah "example.com/old/foo/v1"`
`7`	`7`	`)`
`8`	`8`
`9`	`9`	`func Bar2(baz blah.Baz, addr net.Addr) {}`