From ade9a03a048d77bcf57cabad9977a6034d6c9e1d Mon Sep 17 00:00:00 2001 From: upmorpheus Date: Fri, 13 Oct 2023 15:49:54 +0200 Subject: [PATCH] Correct the order of the assignment of 'err' While reading up on this code to get an example of how util.VisitProtoBytesFields() is used, I noticed this somewhat obvious mistake. We should assign 'copyErr' to 'err'. Not the other way around. --- pkg/cas/blob_access_directory_fetcher.go | 4 +-- pkg/cas/blob_access_directory_fetcher_test.go | 36 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/pkg/cas/blob_access_directory_fetcher.go b/pkg/cas/blob_access_directory_fetcher.go index 47a9fdc..6479611 100644 --- a/pkg/cas/blob_access_directory_fetcher.go +++ b/pkg/cas/blob_access_directory_fetcher.go @@ -69,7 +69,7 @@ func (df *blobAccessDirectoryFetcher) GetTreeRootDirectory(ctx context.Context, return nil }); err != nil { if _, copyErr := io.Copy(io.Discard, r); copyErr != nil { - copyErr = err + err = copyErr } return nil, err } @@ -157,7 +157,7 @@ func (bs *treeBlobSlicer) Slice(b buffer.Buffer, requestedChildDigest digest.Dig bRequested.Discard() } if _, copyErr := io.Copy(io.Discard, r); copyErr != nil { - copyErr = err + err = copyErr } return buffer.NewBufferFromError(err), nil } diff --git a/pkg/cas/blob_access_directory_fetcher_test.go b/pkg/cas/blob_access_directory_fetcher_test.go index 05271e4..ad97910 100644 --- a/pkg/cas/blob_access_directory_fetcher_test.go +++ b/pkg/cas/blob_access_directory_fetcher_test.go @@ -1,7 +1,9 @@ package cas_test import ( + "bytes" "context" + "io" "testing" remoteexecution "github.com/bazelbuild/remote-apis/build/bazel/remote/execution/v2" @@ -115,6 +117,19 @@ func TestBlobAccessDirectoryFetcherGetTreeRootDirectory(t *testing.T) { testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Tree does not contain a root directory"), err) }) + t.Run("ChecksumMismatch", func(t *testing.T) { + // If an REv2 Tree object cannot be parsed, it must be + // read in its entirety to ensure this isn't caused by a + // checksum mismatch. + treeDigest := digest.MustNewDigest("example", remoteexecution.DigestFunction_MD5, "ceb78ab91c6d580aceea6618dd6fc5cc", 10000) + + blobAccess.EXPECT().Get(ctx, treeDigest). + Return(buffer.NewCASBufferFromReader(treeDigest, io.NopCloser(bytes.NewBuffer(make([]byte, 10000))), buffer.UserProvided)) + + _, err := directoryFetcher.GetTreeRootDirectory(ctx, treeDigest) + testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Buffer has checksum b85d6fb9ef4260dcf1ce0a1b0bff80d3, while ceb78ab91c6d580aceea6618dd6fc5cc was expected"), err) + }) + t.Run("Success", func(t *testing.T) { treeDigest := digest.MustNewDigest("example", remoteexecution.DigestFunction_MD5, "f5f634611dd11ccba54c7b9d9607c3c2", 100) exampleDirectory := &remoteexecution.Directory{ @@ -197,6 +212,27 @@ func TestBlobAccessDirectoryFetcherGetTreeChildDirectory(t *testing.T) { testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Field with number 10 at offset 0 has type 4, while 2 was expected"), err) }) + t.Run("ChecksumMismatch", func(t *testing.T) { + // If an REv2 Tree object cannot be parsed, it must be + // read in its entirety to ensure this isn't caused by a + // checksum mismatch. + treeDigest := digest.MustNewDigest("example", remoteexecution.DigestFunction_MD5, "ceb78ab91c6d580aceea6618dd6fc5cc", 10000) + directoryDigest := digest.MustNewDigest("example", remoteexecution.DigestFunction_MD5, "138f65a6fb46dc6d97618a24b4490c19", 10) + + blobAccess.EXPECT().GetFromComposite(ctx, treeDigest, directoryDigest, gomock.Any()). + DoAndReturn(func(ctx context.Context, treeDigest, childDigest digest.Digest, slicer slicing.BlobSlicer) buffer.Buffer { + b, slices := slicer.Slice(buffer.NewCASBufferFromReader(treeDigest, io.NopCloser(bytes.NewBuffer(make([]byte, 10000))), buffer.UserProvided), childDigest) + require.Empty(t, slices) + return b + }) + + _, err := directoryFetcher.GetTreeChildDirectory( + ctx, + treeDigest, + directoryDigest) + testutil.RequireEqualStatus(t, status.Error(codes.InvalidArgument, "Buffer has checksum b85d6fb9ef4260dcf1ce0a1b0bff80d3, while ceb78ab91c6d580aceea6618dd6fc5cc was expected"), err) + }) + t.Run("ValidTree", func(t *testing.T) { // Call GetTreeChildDirectory() against a valid Tree // object. The provided BlobSlicer should be capable of