Merge #63467

63467: filetable: fix NPE when an empty file is downloaded from userfile r=stevendanna a=adityamaru Fixes: #62605 Release note: None Co-authored-by: Aditya Maru <[email protected]>
cockroachdb · Apr 19, 2021 · b8553b7 · b8553b7
2 parents f02b6c8 + c544874
commit b8553b7
Show file tree

Hide file tree

Showing 2 changed files with 51 additions and 26 deletions.
diff --git a/pkg/cli/userfiletable_test.go b/pkg/cli/userfiletable_test.go
@@ -103,6 +103,7 @@ func TestUserFileUpload(t *testing.T) {
 
 	c := NewCLITest(TestCLIParams{T: t})
 	defer c.Cleanup()
+	c.omitArgs = true
 
 	dir, cleanFn := testutils.TempDir(t)
 	defer cleanFn()
@@ -138,37 +139,59 @@ func TestUserFileUpload(t *testing.T) {
 		err := ioutil.WriteFile(filePath, tc.fileContent, 0666)
 		require.NoError(t, err)
 		t.Run(tc.name, func(t *testing.T) {
-			destination := fmt.Sprintf("/test/file%d.csv", i)
+			t.Run("destination-not-full-URI", func(t *testing.T) {
+				destination := fmt.Sprintf("/test/file%d.csv", i)
 
-			_, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath,
-				destination))
-			require.NoError(t, err)
+				_, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath,
+					destination))
+				require.NoError(t, err)
 
-			checkUserFileContent(ctx, t, c.ExecutorConfig(), security.RootUserName(),
-				constructUserfileDestinationURI("", destination, security.RootUserName()),
-				tc.fileContent)
-		})
+				checkUserFileContent(ctx, t, c.ExecutorConfig(), security.RootUserName(),
+					constructUserfileDestinationURI("", destination, security.RootUserName()),
+					tc.fileContent)
+			})
 
-		t.Run(tc.name+"_fullURI", func(t *testing.T) {
-			destination := fmt.Sprintf("userfile://defaultdb.public.foo/test/file%d.csv", i)
-			_, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath,
-				destination))
-			require.NoError(t, err)
+			t.Run("full-URI", func(t *testing.T) {
+				destination := fmt.Sprintf("userfile://defaultdb.public.foo/test/file%d.csv", i)
+				_, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath,
+					destination))
+				require.NoError(t, err)
 
-			checkUserFileContent(ctx, t, c.ExecutorConfig(), security.RootUserName(),
-				destination, tc.fileContent)
-		})
+				checkUserFileContent(ctx, t, c.ExecutorConfig(), security.RootUserName(),
+					destination, tc.fileContent)
+			})
 
-		// Not specifying a qualified table name should default to writing to
-		// `defaultdb.public.userfiles_username`.
-		t.Run(tc.name+"_no-host-uri", func(t *testing.T) {
-			destination := fmt.Sprintf("userfile:///test/file%d.csv", i)
-			_, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath,
-				destination))
-			require.NoError(t, err)
+			// Not specifying a qualified table name should default to writing to
+			// `defaultdb.public.userfiles_username`.
+			t.Run("no-host-uri", func(t *testing.T) {
+				destination := fmt.Sprintf("userfile:///test/nohost/file%d.csv", i)
+				_, err = c.RunWithCapture(fmt.Sprintf("userfile upload %s %s", filePath,
+					destination))
+				require.NoError(t, err)
+
+				checkUserFileContent(ctx, t, c.ExecutorConfig(), security.RootUserName(),
+					destination, tc.fileContent)
+			})
 
-			checkUserFileContent(ctx, t, c.ExecutorConfig(), security.RootUserName(),
-				destination, tc.fileContent)
+			t.Run("get", func(t *testing.T) {
+				dest := filepath.Join(dir, fmt.Sprintf("tc-%d", i))
+				destination := fmt.Sprintf("userfile://defaultdb.public.foo/test/file%d.csv", i)
+				cmd := []string{"userfile", "get", destination, dest}
+				cliOutput, err := c.RunWithCaptureArgs(cmd)
+				require.NoError(t, err)
+				if strings.Contains(cliOutput, "ERROR") {
+					t.Fatalf("unexpected error: %q", cliOutput)
+				} else {
+					lines := strings.Split(strings.TrimSpace(cliOutput), "\n")
+
+					var downloaded []string
+					for i := range lines {
+						downloaded = append(downloaded, strings.Fields(lines[i])[3])
+					}
+					require.Equal(t, []string{fmt.Sprintf("test/file%d.csv", i)}, downloaded,
+						"get files from %v returned %q", cmd, cliOutput)
+				}
+			})
 		})
 	}
 }

diff --git a/pkg/storage/cloudimpl/filetable/file_table_read_writer.go b/pkg/storage/cloudimpl/filetable/file_table_read_writer.go
@@ -697,7 +697,9 @@ func newFileTableReader(
 			return nil, 0, errors.Wrap(err, "failed to read returned file metadata")
 		}
 		fileID = vals[0].([]byte)
-		sz = vals[1].(int64)
+		if vals[1] != nil {
+			sz = vals[1].(int64)
+		}
 	default:
 		panic("unknown executor")
 	}