From 6d88e63f9857d34129152d5bacd3382750e8c102 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Tue, 7 Jul 2020 16:19:51 +0900 Subject: [PATCH 01/13] Add symlink info to file metadata --- go.sum | 2 + go/pkg/filemetadata/cache_posix_test.go | 2 +- go/pkg/filemetadata/cache_test.go | 6 +- go/pkg/filemetadata/filemetadata.go | 33 ++++- go/pkg/filemetadata/filemetadata_test.go | 177 +++++++++++++++++++++-- go/pkg/tree/tree.go | 6 +- 6 files changed, 197 insertions(+), 29 deletions(-) diff --git a/go.sum b/go.sum index ff67409e6..d51ed3c15 100644 --- a/go.sum +++ b/go.sum @@ -32,6 +32,7 @@ github.com/bazelbuild/remote-apis v0.0.0-20190507145712-5556e9c6153f h1:KLZodm2m github.com/bazelbuild/remote-apis v0.0.0-20190507145712-5556e9c6153f/go.mod h1:9Y+1FnaNUGVV6wKE0Jdh+mguqDUsyd9uUqokalrC7DQ= github.com/bazelbuild/remote-apis v0.0.0-20191104140458-e77c4eb2ca48 h1:bgj+Oufa8F4rCHe/8omhml7cBlg3VmNhF66ed1vT2Bw= github.com/bazelbuild/remote-apis v0.0.0-20191104140458-e77c4eb2ca48/go.mod h1:9Y+1FnaNUGVV6wKE0Jdh+mguqDUsyd9uUqokalrC7DQ= +github.com/bazelbuild/remote-apis v0.0.0-20200624085034-0afc3700d177 h1:JeDdP1ZsFOMctOzhcwYfLkGaLT8Nzxc1AXkXKUrWbAw= github.com/boombuler/barcode v1.0.0 h1:s1TvRnXwL2xJRaccrdcBQMZxq6X7DvsMogtmJeHDdrc= github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= github.com/bradfitz/go-smtpd v0.0.0-20170404230938-deb6d6237625 h1:ckJgFhFWywOx+YLEMIJsTb+NV6NexWICk5+AMSuz3ss= @@ -111,6 +112,7 @@ github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.3 h1:gyjaxf+svBWX08ZjK86iN9geUJF0H6gp2IRKX6Nf6/I= github.com/golang/protobuf v1.3.3/go.mod h1:vzj43D7+SQXF/4pzW/hwtAqwc6iTitCiVSaWz5lYuqw= +github.com/golang/protobuf v1.4.2 h1:+Z5KGCizgyZCbGh1KZqA0fcLLkwbsjIzS4aV2v7wJX0= github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db h1:woRePGFeVFfLKN/pOkfl+p/TAqKOfFu+7KPlMVpok/w= github.com/golang/snappy v0.0.0-20180518054509-2e65f85255db/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q= github.com/gonum/blas v0.0.0-20181208220705-f22b278b28ac h1:Q0Jsdxl5jbxouNs1TQYt0gxesYMU4VXRbsTlgDloZ50= diff --git a/go/pkg/filemetadata/cache_posix_test.go b/go/pkg/filemetadata/cache_posix_test.go index 302546d65..4b733ad34 100644 --- a/go/pkg/filemetadata/cache_posix_test.go +++ b/go/pkg/filemetadata/cache_posix_test.go @@ -12,7 +12,7 @@ import ( func TestExecutableCacheLoad(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, true, "") + filename, err := createFile(t, &testFileParams{executable: true}) if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } diff --git a/go/pkg/filemetadata/cache_test.go b/go/pkg/filemetadata/cache_test.go index e331850ef..8af7eaa19 100644 --- a/go/pkg/filemetadata/cache_test.go +++ b/go/pkg/filemetadata/cache_test.go @@ -16,7 +16,7 @@ var ( func TestSimpleCacheLoad(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, false, "") + filename, err := createFile(t, &testFileParams{}) if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } @@ -45,7 +45,7 @@ func TestSimpleCacheLoad(t *testing.T) { func TestCacheOnceLoadMultiple(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, false, "") + filename, err := createFile(t, &testFileParams{}) if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } @@ -75,7 +75,7 @@ func TestCacheOnceLoadMultiple(t *testing.T) { func TestLoadAfterChangeWithoutValidation(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, false, "") + filename, err := createFile(t, &testFileParams{}) if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } diff --git a/go/pkg/filemetadata/filemetadata.go b/go/pkg/filemetadata/filemetadata.go index ccec09835..1c9c1de1d 100644 --- a/go/pkg/filemetadata/filemetadata.go +++ b/go/pkg/filemetadata/filemetadata.go @@ -8,19 +8,25 @@ import ( "github.com/bazelbuild/remote-apis-sdks/go/pkg/digest" ) +// SymlinkMetadata contains details if the given path is a symlink. +type SymlinkMetadata struct { + Target string + IsInvalid bool +} + // Metadata contains details for a particular file. type Metadata struct { Digest digest.Digest IsExecutable bool Err error + Symlink SymlinkMetadata } // FileError is the error returned by the Compute function. type FileError struct { - IsDirectory bool - IsNotFound bool - IsInvalidSymlink bool - Err error + IsDirectory bool + IsNotFound bool + Err error } // Error returns the error message. @@ -41,11 +47,24 @@ func isSymlink(filename string) (bool, error) { func Compute(filename string) *Metadata { md := &Metadata{Digest: digest.Empty} file, err := os.Stat(filename) + if isSym, _ := isSymlink(filename); isSym { + if err != nil { + md.Err = &FileError{Err: err} + md.Symlink.IsInvalid = true + return md + } + dest, err := os.Readlink(filename) + if err != nil { + // This should never happen given that we have verified |filename| is a symlink. + md.Err = &FileError{Err: err} + md.Symlink.IsInvalid = true + return md + } + md.Symlink.Target = dest + } + if err != nil { fe := &FileError{Err: err} - if s, err := isSymlink(filename); err == nil && s { - fe.IsInvalidSymlink = true - } if os.IsNotExist(err) { fe.IsNotFound = true } diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index bc895830e..f965d167b 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -1,37 +1,56 @@ package filemetadata import ( + "crypto/rand" + "encoding/hex" + "errors" + "fmt" "io/ioutil" "os" + "path/filepath" "testing" "github.com/bazelbuild/remote-apis-sdks/go/pkg/digest" "github.com/google/go-cmp/cmp" ) -func TestCompute(t *testing.T) { +type testFileParams struct { + contents string + executable bool +} + +// Used as a type enum +type testDirParams struct { +} + +func TestComputeFiles(t *testing.T) { tests := []struct { - name string - contents string - executable bool + name string + *testFileParams }{ { - name: "empty", - contents: "", + name: "empty", + testFileParams: &testFileParams{ + contents: "", + }, }, { - name: "non-executable", - contents: "bla", + name: "non-executable", + testFileParams: &testFileParams{ + contents: "bla", + }, }, { - name: "executable", - contents: "foo", - executable: true, + name: "executable", + testFileParams: &testFileParams{ + contents: "foo", + executable: true, + }, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - filename, err := createFile(t, tc.executable, tc.contents) + filename, err := createFile(t, tc.testFileParams) if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } @@ -66,10 +85,98 @@ func TestComputeDirectory(t *testing.T) { } } -func createFile(t *testing.T, executable bool, contents string) (string, error) { +type testSymlinkCreationResult struct { + symlink string + target string +} + +func (sc *testSymlinkCreationResult) cleanup() { + os.RemoveAll(sc.symlink) + os.RemoveAll(sc.target) +} + +func TestComputeSymlinks(t *testing.T) { + tests := []struct { + name string + removeTarget bool + target interface{} + }{ + { + name: "valid", + target: &testFileParams{ + contents: "bla", + }, + }, + { + name: "valid-executable", + target: &testFileParams{ + contents: "executable", + executable: true, + }, + }, + { + name: "invalid-file", + removeTarget: true, + }, + { + name: "symlink-dir", + target: &testDirParams{}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + symlinkResult, err := createSymlink(t, tc.target, tc.removeTarget) + if err != nil { + t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) + } + defer symlinkResult.cleanup() + + symlinkPath := symlinkResult.symlink + got := Compute(symlinkPath) + fmt.Printf("symlinkPath=%s got=%+v\n", symlinkPath, got) + + if tc.removeTarget { + if got.Err == nil || !got.Symlink.IsInvalid { + t.Errorf("Compute(%v) should fail because the symlink is invalid", symlinkPath) + } + if got.Symlink.Target != "" { + t.Errorf("Compute(%v) should fail because the symlink is invalid, got target: %s", symlinkPath, got.Symlink.Target) + } + return + } + + if _, ok := tc.target.(*testDirParams); ok { + if fe, ok := got.Err.(*FileError); !ok || !fe.IsDirectory { + t.Errorf("Compute(%v).Err = %v, want FileError{IsDirectory:true}", symlinkPath, got.Err) + } + return + } + + if got.Err != nil { + t.Errorf("Compute(%v) failed. Got error: %v", symlinkPath, got.Err) + } + + fileParams := tc.target.(*testFileParams) + want := &Metadata{ + Symlink: SymlinkMetadata{ + Target: symlinkResult.target, + IsInvalid: false, + }, + Digest: digest.NewFromBlob([]byte(fileParams.contents)), + IsExecutable: fileParams.executable, + } + + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("Compute(%v) returned diff. (-want +got)\n%s", symlinkPath, diff) + } + }) + } +} + +func createFile(t *testing.T, fp *testFileParams) (string, error) { t.Helper() perm := os.FileMode(0666) - if executable { + if fp.executable { perm = os.FileMode(0766) } tmpFile, err := ioutil.TempFile(os.TempDir(), "") @@ -83,8 +190,48 @@ func createFile(t *testing.T, executable bool, contents string) (string, error) return "", err } filename := tmpFile.Name() - if err = ioutil.WriteFile(filename, []byte(contents), os.ModeTemporary); err != nil { + if err = ioutil.WriteFile(filename, []byte(fp.contents), os.ModeTemporary); err != nil { return "", err } return filename, nil } + +func createSymlink(t *testing.T, target interface{}, removeTarget bool) (*testSymlinkCreationResult, error) { + t.Helper() + + if target == nil { + // Create a temporary fake target so that os.Symlink() can work. + target = &testFileParams{ + contents: "transient", + } + } + targetPath, err := func() (string, error) { + switch tgt := target.(type) { + case *testFileParams: + return createFile(t, tgt) + case *testDirParams: + return ioutil.TempDir(os.TempDir(), "") + } + return "", errors.New("Unknown target type") + }() + if err != nil { + return nil, err + } + + randBytes := make([]byte, 16) + rand.Read(randBytes) + symlinkPath := filepath.Join(os.TempDir(), hex.EncodeToString(randBytes)) + if err := os.Symlink(targetPath, symlinkPath); err != nil { + return nil, err + } + + result := &testSymlinkCreationResult{ + symlink: symlinkPath, + } + if removeTarget { + os.RemoveAll(targetPath) + } + result.target = targetPath + + return result, nil +} diff --git a/go/pkg/tree/tree.go b/go/pkg/tree/tree.go index 764f138a2..d9eddc4a6 100644 --- a/go/pkg/tree/tree.go +++ b/go/pkg/tree/tree.go @@ -65,14 +65,14 @@ func loadFiles(execRoot string, excl []*command.InputExclusion, path string, fs absPath := filepath.Clean(filepath.Join(execRoot, path)) meta := cache.Get(absPath) t := command.FileInputType + if meta.Symlink.IsInvalid { + return nil + } if meta.Err != nil { e, ok := meta.Err.(*filemetadata.FileError) if !ok { return meta.Err } - if e.IsInvalidSymlink { - return nil - } if !e.IsDirectory { return meta.Err } From 911fda40252fff2dbe97339799f4785a833b2d2c Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Tue, 7 Jul 2020 17:15:08 +0900 Subject: [PATCH 02/13] use pointer type --- go/pkg/filemetadata/filemetadata.go | 3 ++- go/pkg/tree/tree.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata.go b/go/pkg/filemetadata/filemetadata.go index 1c9c1de1d..371bb544b 100644 --- a/go/pkg/filemetadata/filemetadata.go +++ b/go/pkg/filemetadata/filemetadata.go @@ -19,7 +19,7 @@ type Metadata struct { Digest digest.Digest IsExecutable bool Err error - Symlink SymlinkMetadata + Symlink *SymlinkMetadata } // FileError is the error returned by the Compute function. @@ -48,6 +48,7 @@ func Compute(filename string) *Metadata { md := &Metadata{Digest: digest.Empty} file, err := os.Stat(filename) if isSym, _ := isSymlink(filename); isSym { + md.Symlink = &SymlinkMetadata{} if err != nil { md.Err = &FileError{Err: err} md.Symlink.IsInvalid = true diff --git a/go/pkg/tree/tree.go b/go/pkg/tree/tree.go index d9eddc4a6..88bbf00a5 100644 --- a/go/pkg/tree/tree.go +++ b/go/pkg/tree/tree.go @@ -65,7 +65,7 @@ func loadFiles(execRoot string, excl []*command.InputExclusion, path string, fs absPath := filepath.Clean(filepath.Join(execRoot, path)) meta := cache.Get(absPath) t := command.FileInputType - if meta.Symlink.IsInvalid { + if smd := meta.Symlink; smd != nil && smd.IsInvalid { return nil } if meta.Err != nil { From 85722c78fd08df430e59da9a37da684978361e4b Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Tue, 7 Jul 2020 17:23:14 +0900 Subject: [PATCH 03/13] simplify --- go/pkg/filemetadata/filemetadata_test.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index f965d167b..8011c19bb 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -97,9 +97,8 @@ func (sc *testSymlinkCreationResult) cleanup() { func TestComputeSymlinks(t *testing.T) { tests := []struct { - name string - removeTarget bool - target interface{} + name string + target interface{} // If unspecified, this is an invalid symlink }{ { name: "valid", @@ -115,8 +114,7 @@ func TestComputeSymlinks(t *testing.T) { }, }, { - name: "invalid-file", - removeTarget: true, + name: "invalid-file", }, { name: "symlink-dir", @@ -125,7 +123,7 @@ func TestComputeSymlinks(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - symlinkResult, err := createSymlink(t, tc.target, tc.removeTarget) + symlinkResult, err := createSymlink(t, tc.target) if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } @@ -135,7 +133,7 @@ func TestComputeSymlinks(t *testing.T) { got := Compute(symlinkPath) fmt.Printf("symlinkPath=%s got=%+v\n", symlinkPath, got) - if tc.removeTarget { + if tc.target == nil { if got.Err == nil || !got.Symlink.IsInvalid { t.Errorf("Compute(%v) should fail because the symlink is invalid", symlinkPath) } @@ -158,7 +156,7 @@ func TestComputeSymlinks(t *testing.T) { fileParams := tc.target.(*testFileParams) want := &Metadata{ - Symlink: SymlinkMetadata{ + Symlink: &SymlinkMetadata{ Target: symlinkResult.target, IsInvalid: false, }, @@ -196,10 +194,11 @@ func createFile(t *testing.T, fp *testFileParams) (string, error) { return filename, nil } -func createSymlink(t *testing.T, target interface{}, removeTarget bool) (*testSymlinkCreationResult, error) { +func createSymlink(t *testing.T, target interface{}) (*testSymlinkCreationResult, error) { t.Helper() - if target == nil { + invalidTarget := target == nil + if invalidTarget { // Create a temporary fake target so that os.Symlink() can work. target = &testFileParams{ contents: "transient", @@ -228,7 +227,7 @@ func createSymlink(t *testing.T, target interface{}, removeTarget bool) (*testSy result := &testSymlinkCreationResult{ symlink: symlinkPath, } - if removeTarget { + if invalidTarget { os.RemoveAll(targetPath) } result.target = targetPath From 8743bc74d8a87c17eec446d1873301e2e788cc5c Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Tue, 7 Jul 2020 17:26:29 +0900 Subject: [PATCH 04/13] remove print --- go/pkg/filemetadata/filemetadata_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index 8011c19bb..416557e55 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -4,7 +4,6 @@ import ( "crypto/rand" "encoding/hex" "errors" - "fmt" "io/ioutil" "os" "path/filepath" @@ -131,7 +130,6 @@ func TestComputeSymlinks(t *testing.T) { symlinkPath := symlinkResult.symlink got := Compute(symlinkPath) - fmt.Printf("symlinkPath=%s got=%+v\n", symlinkPath, got) if tc.target == nil { if got.Err == nil || !got.Symlink.IsInvalid { From b9faa1d2b9fbc2e750a22bccc9cabc7e358af9f0 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Wed, 8 Jul 2020 09:51:06 +0900 Subject: [PATCH 05/13] IsDangling --- go/pkg/filemetadata/filemetadata.go | 6 +++--- go/pkg/filemetadata/filemetadata_test.go | 4 ++-- go/pkg/tree/tree.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata.go b/go/pkg/filemetadata/filemetadata.go index 371bb544b..e188b56b1 100644 --- a/go/pkg/filemetadata/filemetadata.go +++ b/go/pkg/filemetadata/filemetadata.go @@ -11,7 +11,7 @@ import ( // SymlinkMetadata contains details if the given path is a symlink. type SymlinkMetadata struct { Target string - IsInvalid bool + IsDangling bool } // Metadata contains details for a particular file. @@ -51,14 +51,14 @@ func Compute(filename string) *Metadata { md.Symlink = &SymlinkMetadata{} if err != nil { md.Err = &FileError{Err: err} - md.Symlink.IsInvalid = true + md.Symlink.IsDangling = true return md } dest, err := os.Readlink(filename) if err != nil { // This should never happen given that we have verified |filename| is a symlink. md.Err = &FileError{Err: err} - md.Symlink.IsInvalid = true + md.Symlink.IsDangling = true return md } md.Symlink.Target = dest diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index 416557e55..727e69f7d 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -132,7 +132,7 @@ func TestComputeSymlinks(t *testing.T) { got := Compute(symlinkPath) if tc.target == nil { - if got.Err == nil || !got.Symlink.IsInvalid { + if got.Err == nil || !got.Symlink.IsDangling { t.Errorf("Compute(%v) should fail because the symlink is invalid", symlinkPath) } if got.Symlink.Target != "" { @@ -156,7 +156,7 @@ func TestComputeSymlinks(t *testing.T) { want := &Metadata{ Symlink: &SymlinkMetadata{ Target: symlinkResult.target, - IsInvalid: false, + IsDangling: false, }, Digest: digest.NewFromBlob([]byte(fileParams.contents)), IsExecutable: fileParams.executable, diff --git a/go/pkg/tree/tree.go b/go/pkg/tree/tree.go index 88bbf00a5..f7155ab9e 100644 --- a/go/pkg/tree/tree.go +++ b/go/pkg/tree/tree.go @@ -65,7 +65,7 @@ func loadFiles(execRoot string, excl []*command.InputExclusion, path string, fs absPath := filepath.Clean(filepath.Join(execRoot, path)) meta := cache.Get(absPath) t := command.FileInputType - if smd := meta.Symlink; smd != nil && smd.IsInvalid { + if smd := meta.Symlink; smd != nil && smd.IsDangling { return nil } if meta.Err != nil { From e3df0d08a1c41eef50b30a18206f7d701519f1c2 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Wed, 8 Jul 2020 10:21:20 +0900 Subject: [PATCH 06/13] fix tests --- go/pkg/filemetadata/filemetadata_test.go | 115 +++++++++++------------ 1 file changed, 57 insertions(+), 58 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index 727e69f7d..4841d3a7d 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -3,7 +3,6 @@ package filemetadata import ( "crypto/rand" "encoding/hex" - "errors" "io/ioutil" "os" "path/filepath" @@ -94,10 +93,10 @@ func (sc *testSymlinkCreationResult) cleanup() { os.RemoveAll(sc.target) } -func TestComputeSymlinks(t *testing.T) { +func TestComputeSymlinksToFile(t *testing.T) { tests := []struct { name string - target interface{} // If unspecified, this is an invalid symlink + target *testFileParams }{ { name: "valid", @@ -112,17 +111,11 @@ func TestComputeSymlinks(t *testing.T) { executable: true, }, }, - { - name: "invalid-file", - }, - { - name: "symlink-dir", - target: &testDirParams{}, - }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - symlinkResult, err := createSymlink(t, tc.target) + fileParams := tc.target + symlinkResult, err := createSymlinkTofile(t, fileParams) if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } @@ -131,31 +124,12 @@ func TestComputeSymlinks(t *testing.T) { symlinkPath := symlinkResult.symlink got := Compute(symlinkPath) - if tc.target == nil { - if got.Err == nil || !got.Symlink.IsDangling { - t.Errorf("Compute(%v) should fail because the symlink is invalid", symlinkPath) - } - if got.Symlink.Target != "" { - t.Errorf("Compute(%v) should fail because the symlink is invalid, got target: %s", symlinkPath, got.Symlink.Target) - } - return - } - - if _, ok := tc.target.(*testDirParams); ok { - if fe, ok := got.Err.(*FileError); !ok || !fe.IsDirectory { - t.Errorf("Compute(%v).Err = %v, want FileError{IsDirectory:true}", symlinkPath, got.Err) - } - return - } - if got.Err != nil { t.Errorf("Compute(%v) failed. Got error: %v", symlinkPath, got.Err) } - - fileParams := tc.target.(*testFileParams) want := &Metadata{ Symlink: &SymlinkMetadata{ - Target: symlinkResult.target, + Target: symlinkResult.target, IsDangling: false, }, Digest: digest.NewFromBlob([]byte(fileParams.contents)), @@ -169,6 +143,48 @@ func TestComputeSymlinks(t *testing.T) { } } +func TestComputeDanglingSymlinks(t *testing.T) { + // Create a temporary fake target so that os.Symlink() can work. + symlinkResult, err := createSymlinkTofile(t, &testFileParams{ + contents: "transient", + }) + if err != nil { + t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) + } + // Make the symlink dangling + os.RemoveAll(symlinkResult.target) + defer symlinkResult.cleanup() + + symlinkPath := symlinkResult.symlink + got := Compute(symlinkPath) + if got.Err == nil || !got.Symlink.IsDangling { + t.Errorf("Compute(%v) should fail because the symlink is invalid", symlinkPath) + } + if got.Symlink.Target != "" { + t.Errorf("Compute(%v) should fail because the symlink is invalid, got target: %s", symlinkPath, got.Symlink.Target) + } +} + +func TestComputeSymlinksToDirectory(t *testing.T) { + targetPath, err := ioutil.TempDir(os.TempDir(), "") + if err != nil { + t.Fatalf("Failed to create testing directory: %v", err) + } + symlinkResult, err := createSymlinkToTarget(t, targetPath) + + if err != nil { + t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) + } + // Make the symlink dangling + defer symlinkResult.cleanup() + + symlinkPath := symlinkResult.symlink + got := Compute(symlinkPath) + if fe, ok := got.Err.(*FileError); !ok || !fe.IsDirectory { + t.Errorf("Compute(%v).Err = %v, want FileError{IsDirectory:true}", symlinkPath, got.Err) + } +} + func createFile(t *testing.T, fp *testFileParams) (string, error) { t.Helper() perm := os.FileMode(0666) @@ -192,28 +208,16 @@ func createFile(t *testing.T, fp *testFileParams) (string, error) { return filename, nil } -func createSymlink(t *testing.T, target interface{}) (*testSymlinkCreationResult, error) { - t.Helper() - - invalidTarget := target == nil - if invalidTarget { - // Create a temporary fake target so that os.Symlink() can work. - target = &testFileParams{ - contents: "transient", - } - } - targetPath, err := func() (string, error) { - switch tgt := target.(type) { - case *testFileParams: - return createFile(t, tgt) - case *testDirParams: - return ioutil.TempDir(os.TempDir(), "") - } - return "", errors.New("Unknown target type") - }() +func createSymlinkTofile(t *testing.T, target *testFileParams) (*testSymlinkCreationResult, error) { + targetPath, err := createFile(t, target) if err != nil { return nil, err } + return createSymlinkToTarget(t, targetPath) +} + +func createSymlinkToTarget(t *testing.T, targetPath string) (*testSymlinkCreationResult, error) { + t.Helper() randBytes := make([]byte, 16) rand.Read(randBytes) @@ -222,13 +226,8 @@ func createSymlink(t *testing.T, target interface{}) (*testSymlinkCreationResult return nil, err } - result := &testSymlinkCreationResult{ + return &testSymlinkCreationResult{ symlink: symlinkPath, - } - if invalidTarget { - os.RemoveAll(targetPath) - } - result.target = targetPath - - return result, nil + target: targetPath, + }, nil } From 552b0f1133e31ea785743cf9b2ef80a2214a4697 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Wed, 8 Jul 2020 10:23:07 +0900 Subject: [PATCH 07/13] fmt --- go/pkg/filemetadata/filemetadata.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/pkg/filemetadata/filemetadata.go b/go/pkg/filemetadata/filemetadata.go index e188b56b1..f3f0eed69 100644 --- a/go/pkg/filemetadata/filemetadata.go +++ b/go/pkg/filemetadata/filemetadata.go @@ -10,7 +10,7 @@ import ( // SymlinkMetadata contains details if the given path is a symlink. type SymlinkMetadata struct { - Target string + Target string IsDangling bool } From 97768db09af666ba9ede43d2ea325a7f769e2d92 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Wed, 8 Jul 2020 10:24:51 +0900 Subject: [PATCH 08/13] rm unused --- go/pkg/filemetadata/filemetadata_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index 4841d3a7d..de6d1200a 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -175,7 +175,6 @@ func TestComputeSymlinksToDirectory(t *testing.T) { if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } - // Make the symlink dangling defer symlinkResult.cleanup() symlinkPath := symlinkResult.symlink From f724e1cab5e0a6ce41db10ad63a4b02558d7fde4 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Wed, 8 Jul 2020 10:28:19 +0900 Subject: [PATCH 09/13] fix msg --- go/pkg/filemetadata/filemetadata_test.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index de6d1200a..58dcba6bc 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -17,10 +17,6 @@ type testFileParams struct { executable bool } -// Used as a type enum -type testDirParams struct { -} - func TestComputeFiles(t *testing.T) { tests := []struct { name string @@ -158,17 +154,17 @@ func TestComputeDanglingSymlinks(t *testing.T) { symlinkPath := symlinkResult.symlink got := Compute(symlinkPath) if got.Err == nil || !got.Symlink.IsDangling { - t.Errorf("Compute(%v) should fail because the symlink is invalid", symlinkPath) + t.Errorf("Compute(%v) should fail because the symlink is dangling", symlinkPath) } if got.Symlink.Target != "" { - t.Errorf("Compute(%v) should fail because the symlink is invalid, got target: %s", symlinkPath, got.Symlink.Target) + t.Errorf("Compute(%v) should fail because the symlink is dangling, got target: %s", symlinkPath, got.Symlink.Target) } } func TestComputeSymlinksToDirectory(t *testing.T) { targetPath, err := ioutil.TempDir(os.TempDir(), "") if err != nil { - t.Fatalf("Failed to create testing directory: %v", err) + t.Fatalf("Failed to create tmp directory: %v", err) } symlinkResult, err := createSymlinkToTarget(t, targetPath) From 447e40bedd65955411106aeb359f13cc73dde3c7 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Wed, 8 Jul 2020 14:59:42 +0900 Subject: [PATCH 10/13] set target even if dangling --- go/pkg/filemetadata/filemetadata.go | 12 ++++++------ go/pkg/filemetadata/filemetadata_test.go | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata.go b/go/pkg/filemetadata/filemetadata.go index f3f0eed69..e3bb5c83c 100644 --- a/go/pkg/filemetadata/filemetadata.go +++ b/go/pkg/filemetadata/filemetadata.go @@ -49,19 +49,19 @@ func Compute(filename string) *Metadata { file, err := os.Stat(filename) if isSym, _ := isSymlink(filename); isSym { md.Symlink = &SymlinkMetadata{} - if err != nil { - md.Err = &FileError{Err: err} - md.Symlink.IsDangling = true + if dest, rlErr := os.Readlink(filename); rlErr != nil { + md.Err = &FileError{Err: rlErr} return md + } else { + // If Readlink was OK, we set Target, even if this could be a dangling symlink. + md.Symlink.Target = dest } - dest, err := os.Readlink(filename) + if err != nil { - // This should never happen given that we have verified |filename| is a symlink. md.Err = &FileError{Err: err} md.Symlink.IsDangling = true return md } - md.Symlink.Target = dest } if err != nil { diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index 58dcba6bc..9f5954fd6 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -156,8 +156,8 @@ func TestComputeDanglingSymlinks(t *testing.T) { if got.Err == nil || !got.Symlink.IsDangling { t.Errorf("Compute(%v) should fail because the symlink is dangling", symlinkPath) } - if got.Symlink.Target != "" { - t.Errorf("Compute(%v) should fail because the symlink is dangling, got target: %s", symlinkPath, got.Symlink.Target) + if got.Symlink.Target != symlinkResult.target { + t.Errorf("Compute(%v) should still set Target for the dangling symlink, want=%v got=%v", symlinkPath, symlinkResult.target, got.Symlink.Target) } } From 9d3ac5612e904504e290bb0724c8cb01c0a833d1 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Thu, 9 Jul 2020 09:17:45 +0900 Subject: [PATCH 11/13] apply fix --- go/pkg/filemetadata/filemetadata_test.go | 64 ++++++++---------------- 1 file changed, 22 insertions(+), 42 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index 9f5954fd6..76011561a 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -1,8 +1,6 @@ package filemetadata import ( - "crypto/rand" - "encoding/hex" "io/ioutil" "os" "path/filepath" @@ -79,16 +77,6 @@ func TestComputeDirectory(t *testing.T) { } } -type testSymlinkCreationResult struct { - symlink string - target string -} - -func (sc *testSymlinkCreationResult) cleanup() { - os.RemoveAll(sc.symlink) - os.RemoveAll(sc.target) -} - func TestComputeSymlinksToFile(t *testing.T) { tests := []struct { name string @@ -111,13 +99,12 @@ func TestComputeSymlinksToFile(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { fileParams := tc.target - symlinkResult, err := createSymlinkTofile(t, fileParams) + symlinkPath := filepath.Join(os.TempDir(), tc.name) + defer os.RemoveAll(symlinkPath) + targetPath, err := createSymlinkTofile(t, symlinkPath, fileParams) if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } - defer symlinkResult.cleanup() - - symlinkPath := symlinkResult.symlink got := Compute(symlinkPath) if got.Err != nil { @@ -125,7 +112,7 @@ func TestComputeSymlinksToFile(t *testing.T) { } want := &Metadata{ Symlink: &SymlinkMetadata{ - Target: symlinkResult.target, + Target: targetPath, IsDangling: false, }, Digest: digest.NewFromBlob([]byte(fileParams.contents)), @@ -141,39 +128,39 @@ func TestComputeSymlinksToFile(t *testing.T) { func TestComputeDanglingSymlinks(t *testing.T) { // Create a temporary fake target so that os.Symlink() can work. - symlinkResult, err := createSymlinkTofile(t, &testFileParams{ + symlinkPath := filepath.Join(os.TempDir(), "dangling") + defer os.RemoveAll(symlinkPath) + targetPath, err := createSymlinkTofile(t, symlinkPath, &testFileParams{ contents: "transient", }) if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } // Make the symlink dangling - os.RemoveAll(symlinkResult.target) - defer symlinkResult.cleanup() + os.RemoveAll(targetPath) - symlinkPath := symlinkResult.symlink got := Compute(symlinkPath) if got.Err == nil || !got.Symlink.IsDangling { t.Errorf("Compute(%v) should fail because the symlink is dangling", symlinkPath) } - if got.Symlink.Target != symlinkResult.target { - t.Errorf("Compute(%v) should still set Target for the dangling symlink, want=%v got=%v", symlinkPath, symlinkResult.target, got.Symlink.Target) + if got.Symlink.Target != targetPath { + t.Errorf("Compute(%v) should still set Target for the dangling symlink, want=%v got=%v", symlinkPath, targetPath, got.Symlink.Target) } } func TestComputeSymlinksToDirectory(t *testing.T) { + symlinkPath := filepath.Join(os.TempDir(), "dir-symlink") + defer os.RemoveAll(symlinkPath) targetPath, err := ioutil.TempDir(os.TempDir(), "") if err != nil { t.Fatalf("Failed to create tmp directory: %v", err) } - symlinkResult, err := createSymlinkToTarget(t, targetPath) + err = createSymlinkToTarget(t, symlinkPath, targetPath) if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } - defer symlinkResult.cleanup() - symlinkPath := symlinkResult.symlink got := Compute(symlinkPath) if fe, ok := got.Err.(*FileError); !ok || !fe.IsDirectory { t.Errorf("Compute(%v).Err = %v, want FileError{IsDirectory:true}", symlinkPath, got.Err) @@ -203,26 +190,19 @@ func createFile(t *testing.T, fp *testFileParams) (string, error) { return filename, nil } -func createSymlinkTofile(t *testing.T, target *testFileParams) (*testSymlinkCreationResult, error) { +func createSymlinkTofile(t *testing.T, symlinkPath string, target *testFileParams) (string, error) { + t.Helper() targetPath, err := createFile(t, target) if err != nil { - return nil, err + return "", err + } + if err := createSymlinkToTarget(t, symlinkPath, targetPath); err != nil { + return "", err } - return createSymlinkToTarget(t, targetPath) + return targetPath, nil } -func createSymlinkToTarget(t *testing.T, targetPath string) (*testSymlinkCreationResult, error) { +func createSymlinkToTarget(t *testing.T, symlinkPath string, targetPath string) error { t.Helper() - - randBytes := make([]byte, 16) - rand.Read(randBytes) - symlinkPath := filepath.Join(os.TempDir(), hex.EncodeToString(randBytes)) - if err := os.Symlink(targetPath, symlinkPath); err != nil { - return nil, err - } - - return &testSymlinkCreationResult{ - symlink: symlinkPath, - target: targetPath, - }, nil + return os.Symlink(targetPath, symlinkPath) } From 32aa7fcc90bb9ac898439c9f14364837bd14a7dd Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Thu, 9 Jul 2020 09:22:11 +0900 Subject: [PATCH 12/13] func name --- go/pkg/filemetadata/filemetadata_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index 76011561a..f3c821068 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -101,7 +101,7 @@ func TestComputeSymlinksToFile(t *testing.T) { fileParams := tc.target symlinkPath := filepath.Join(os.TempDir(), tc.name) defer os.RemoveAll(symlinkPath) - targetPath, err := createSymlinkTofile(t, symlinkPath, fileParams) + targetPath, err := createSymlinkToFile(t, symlinkPath, fileParams) if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } @@ -130,7 +130,7 @@ func TestComputeDanglingSymlinks(t *testing.T) { // Create a temporary fake target so that os.Symlink() can work. symlinkPath := filepath.Join(os.TempDir(), "dangling") defer os.RemoveAll(symlinkPath) - targetPath, err := createSymlinkTofile(t, symlinkPath, &testFileParams{ + targetPath, err := createSymlinkToFile(t, symlinkPath, &testFileParams{ contents: "transient", }) if err != nil { @@ -190,7 +190,7 @@ func createFile(t *testing.T, fp *testFileParams) (string, error) { return filename, nil } -func createSymlinkTofile(t *testing.T, symlinkPath string, target *testFileParams) (string, error) { +func createSymlinkToFile(t *testing.T, symlinkPath string, target *testFileParams) (string, error) { t.Helper() targetPath, err := createFile(t, target) if err != nil { From 12076a277073a62c74b6779793f9eb0c3092b5d9 Mon Sep 17 00:00:00 2001 From: Ye Kuang Date: Tue, 14 Jul 2020 18:03:17 +0900 Subject: [PATCH 13/13] rm testFileParams --- go/pkg/filemetadata/cache_posix_test.go | 2 +- go/pkg/filemetadata/cache_test.go | 6 +- go/pkg/filemetadata/filemetadata_test.go | 72 +++++++++--------------- 3 files changed, 32 insertions(+), 48 deletions(-) diff --git a/go/pkg/filemetadata/cache_posix_test.go b/go/pkg/filemetadata/cache_posix_test.go index 4b733ad34..302546d65 100644 --- a/go/pkg/filemetadata/cache_posix_test.go +++ b/go/pkg/filemetadata/cache_posix_test.go @@ -12,7 +12,7 @@ import ( func TestExecutableCacheLoad(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, &testFileParams{executable: true}) + filename, err := createFile(t, true, "") if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } diff --git a/go/pkg/filemetadata/cache_test.go b/go/pkg/filemetadata/cache_test.go index 8af7eaa19..e331850ef 100644 --- a/go/pkg/filemetadata/cache_test.go +++ b/go/pkg/filemetadata/cache_test.go @@ -16,7 +16,7 @@ var ( func TestSimpleCacheLoad(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, &testFileParams{}) + filename, err := createFile(t, false, "") if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } @@ -45,7 +45,7 @@ func TestSimpleCacheLoad(t *testing.T) { func TestCacheOnceLoadMultiple(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, &testFileParams{}) + filename, err := createFile(t, false, "") if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } @@ -75,7 +75,7 @@ func TestCacheOnceLoadMultiple(t *testing.T) { func TestLoadAfterChangeWithoutValidation(t *testing.T) { c := NewSingleFlightCache() - filename, err := createFile(t, &testFileParams{}) + filename, err := createFile(t, false, "") if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } diff --git a/go/pkg/filemetadata/filemetadata_test.go b/go/pkg/filemetadata/filemetadata_test.go index f3c821068..b7ffc70ee 100644 --- a/go/pkg/filemetadata/filemetadata_test.go +++ b/go/pkg/filemetadata/filemetadata_test.go @@ -10,39 +10,29 @@ import ( "github.com/google/go-cmp/cmp" ) -type testFileParams struct { - contents string - executable bool -} - func TestComputeFiles(t *testing.T) { tests := []struct { - name string - *testFileParams + name string + contents string + executable bool }{ { - name: "empty", - testFileParams: &testFileParams{ - contents: "", - }, + name: "empty", + contents: "", }, { - name: "non-executable", - testFileParams: &testFileParams{ - contents: "bla", - }, + name: "non-executable", + contents: "bla", }, { - name: "executable", - testFileParams: &testFileParams{ - contents: "foo", - executable: true, - }, + name: "executable", + contents: "foo", + executable: true, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - filename, err := createFile(t, tc.testFileParams) + filename, err := createFile(t, tc.executable, tc.contents) if err != nil { t.Fatalf("Failed to create tmp file for testing digests: %v", err) } @@ -79,29 +69,25 @@ func TestComputeDirectory(t *testing.T) { func TestComputeSymlinksToFile(t *testing.T) { tests := []struct { - name string - target *testFileParams + name string + contents string + executable bool }{ { - name: "valid", - target: &testFileParams{ - contents: "bla", - }, + name: "valid", + contents: "bla", }, { - name: "valid-executable", - target: &testFileParams{ - contents: "executable", - executable: true, - }, + name: "valid-executable", + contents: "executable", + executable: true, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - fileParams := tc.target symlinkPath := filepath.Join(os.TempDir(), tc.name) defer os.RemoveAll(symlinkPath) - targetPath, err := createSymlinkToFile(t, symlinkPath, fileParams) + targetPath, err := createSymlinkToFile(t, symlinkPath, tc.executable, tc.contents) if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } @@ -115,8 +101,8 @@ func TestComputeSymlinksToFile(t *testing.T) { Target: targetPath, IsDangling: false, }, - Digest: digest.NewFromBlob([]byte(fileParams.contents)), - IsExecutable: fileParams.executable, + Digest: digest.NewFromBlob([]byte(tc.contents)), + IsExecutable: tc.executable, } if diff := cmp.Diff(want, got); diff != "" { @@ -130,9 +116,7 @@ func TestComputeDanglingSymlinks(t *testing.T) { // Create a temporary fake target so that os.Symlink() can work. symlinkPath := filepath.Join(os.TempDir(), "dangling") defer os.RemoveAll(symlinkPath) - targetPath, err := createSymlinkToFile(t, symlinkPath, &testFileParams{ - contents: "transient", - }) + targetPath, err := createSymlinkToFile(t, symlinkPath, false, "transient") if err != nil { t.Fatalf("Failed to create tmp symlink for testing digests: %v", err) } @@ -167,10 +151,10 @@ func TestComputeSymlinksToDirectory(t *testing.T) { } } -func createFile(t *testing.T, fp *testFileParams) (string, error) { +func createFile(t *testing.T, executable bool, contents string) (string, error) { t.Helper() perm := os.FileMode(0666) - if fp.executable { + if executable { perm = os.FileMode(0766) } tmpFile, err := ioutil.TempFile(os.TempDir(), "") @@ -184,15 +168,15 @@ func createFile(t *testing.T, fp *testFileParams) (string, error) { return "", err } filename := tmpFile.Name() - if err = ioutil.WriteFile(filename, []byte(fp.contents), os.ModeTemporary); err != nil { + if err = ioutil.WriteFile(filename, []byte(contents), os.ModeTemporary); err != nil { return "", err } return filename, nil } -func createSymlinkToFile(t *testing.T, symlinkPath string, target *testFileParams) (string, error) { +func createSymlinkToFile(t *testing.T, symlinkPath string, executable bool, contents string) (string, error) { t.Helper() - targetPath, err := createFile(t, target) + targetPath, err := createFile(t, executable, contents) if err != nil { return "", err }