Merge #59848

59848: cloudimpl: rework userfile listing’s prefix/pattern matching r=dt a=dt

This updates the logic used by userfile’s ListFiles w.r.t.
how it choses the prefix of the table to search and how it
filters results using the supplied pattern.

As before, we list the underlying store using some prefix. 
However whereas previously this was always the prefix configured
for the storage connection, or the prefix of it until the first
glob-pattern character, it now is derived from that or additionally
from the prefix of the passed in pattern-suffix up to the first 
pattern character. Thus a prefix of “/a” and a pattern of “b/c/d*.txt”
would now list “/a/b/c/d” instead of “/a”. 

This is obviously more efficient but also makes the behavior more 
consistent, so that listing “/a/b/c” on a connection with configured
prefix of “/“ is now the same as listing “/c“ on a connection with a 
configured prefix of “/a/b”. 

As before, if a returned path is contained by the connection’s prefix,
which it will be any time the connection prefix does not include a pattern,
the relative path, i.e. the suffix, is what is returned, otherwise the 
fully-qualified path is returned.

This logic currently only added to userfile, but is pulled into two helpers
which can later be reused in the other ListFiles methods to make this behavior
more consistent across implementations. 

Release note: none.

Co-authored-by: David Taylor <[email protected]>

pkg/cli/userfile.go

@@ -24,7 +24,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/storage/cloud"
 	"github.com/cockroachdb/cockroach/pkg/storage/cloudimpl"
 	"github.com/cockroachdb/errors"
@@ -69,7 +68,7 @@ var userFileDeleteCmd = &cobra.Command{
 Deletes the files stored in the user scoped file storage which match the provided pattern,
 using a SQL connection. If passed pattern '*', all files in the specified
 (or default, if unspecified) user scoped file storage will be deleted. Deletions are not
-atomic, and all deletions prior to the first failure will occur. 
+atomic, and all deletions prior to the first failure will occur.
 `,
 	Args:    cobra.MinimumNArgs(1),
 	RunE:    maybeShoutError(runUserFileDelete),
@@ -281,14 +280,16 @@ func listUserFile(ctx context.Context, conn *sqlConn, glob string) ([]string, er
 	if err != nil {
 		return nil, err
 	}
+	prefix := userFileTableConf.FileTableConfig.Path
+	userFileTableConf.FileTableConfig.Path = ""
 
 	f, err := cloudimpl.MakeSQLConnFileTableStorage(ctx, userFileTableConf.FileTableConfig,
 		conn.conn.(cloud.SQLConnI))
 	if err != nil {
 		return nil, err
 	}
 
-	return f.ListFiles(ctx, "")
+	return f.ListFiles(ctx, prefix)
 }
 
 func deleteUserFile(ctx context.Context, conn *sqlConn, glob string) ([]string, error) {
@@ -328,42 +329,18 @@ func deleteUserFile(ctx context.Context, conn *sqlConn, glob string) ([]string,
 	if err != nil {
 		return nil, err
 	}
+
 	files, err := f.ListFiles(ctx, userfileParsedURL.Path)
 	if err != nil {
 		return nil, err
 	}
 
-	var deletedFiles []string
-	for _, file := range files {
-		var deleteFileBasename string
-		if userfileParsedURL.Path == "" {
-			// ListFiles will return absolute userfile URIs which will require
-			// parsing.
-			parsedFile, err := url.ParseRequestURI(file)
-			if err != nil {
-				return deletedFiles, errors.WithDetailf(err, "deletion failed at %s", file)
-			}
-			deleteFileBasename = parsedFile.Path
-		} else {
-			// ListFiles returns relative filepaths without a leading /. All files are
-			// stored with a prefix / in the underlying user scoped tables.
-			deleteFileBasename = path.Join("/", file)
-		}
-		err = f.Delete(ctx, deleteFileBasename)
-		if err != nil {
-			return deletedFiles, errors.WithDetail(err, fmt.Sprintf("deletion failed at %s", file))
-		}
-
-		composedTableName := tree.Name(cloudimpl.DefaultQualifiedNamePrefix + connURL.User.Username())
-		resolvedHost := cloudimpl.DefaultQualifiedNamespace +
-			// Escape special identifiers as needed.
-			composedTableName.String()
-		if userfileParsedURL.Host != "" {
-			resolvedHost = userfileParsedURL.Host
+	for i := range files {
+		if err = f.Delete(ctx, files[i]); err != nil {
+			return files[:i], errors.WithDetailf(err, "deletion failed at %s", files[i])
 		}
-		deletedFiles = append(deletedFiles, fmt.Sprintf("userfile://%s%s", resolvedHost, deleteFileBasename))
 	}
-	return deletedFiles, nil
+	return files, nil
 }
 
 func renameUserFile(

pkg/cli/userfiletable_test.go

@@ -174,7 +174,8 @@ func TestUserFileUpload(t *testing.T) {
 }
 
 func checkListedFiles(t *testing.T, c cliTest, uri string, args string, expectedFiles []string) {
-	cliOutput, err := c.RunWithCaptureArgs([]string{"userfile", "list", uri, args})
+	cmd := []string{"userfile", "list", uri, args}
+	cliOutput, err := c.RunWithCaptureArgs(cmd)
 	require.NoError(t, err)
 	cliOutput = strings.TrimSpace(cliOutput)
 
@@ -184,11 +185,12 @@ func checkListedFiles(t *testing.T, c cliTest, uri string, args string, expected
 		listedFiles = strings.Split(cliOutput, "\n")
 	}
 
-	require.Equal(t, expectedFiles, listedFiles)
+	require.Equal(t, expectedFiles, listedFiles, "listed files from %v", cmd)
 }
 
 func checkDeletedFiles(t *testing.T, c cliTest, uri, args string, expectedFiles []string) {
-	cliOutput, err := c.RunWithCaptureArgs([]string{"userfile", "delete", uri, args})
+	cmd := []string{"userfile", "delete", uri, args}
+	cliOutput, err := c.RunWithCaptureArgs(cmd)
 	require.NoError(t, err)
 	cliOutput = strings.TrimSpace(cliOutput)
 
@@ -201,7 +203,7 @@ func checkDeletedFiles(t *testing.T, c cliTest, uri, args string, expectedFiles
 		}
 	}
 
-	require.Equal(t, expectedFiles, deletedFiles)
+	require.Equal(t, expectedFiles, deletedFiles, "deleted files when running %v", cmd)
 }
 
 func TestUserFileList(t *testing.T) {
@@ -230,14 +232,6 @@ func TestUserFileList(t *testing.T) {
 		Host:   defaultQualifiedNamePrefix + security.RootUser,
 	}
 
-	abs := func(in []string) []string {
-		out := make([]string, len(in))
-		for i := range in {
-			out[i] = defaultUserfileURLSchemeAndHost.String() + "/" + in[i]
-		}
-		return out
-	}
-
 	t.Run("ListFiles", func(t *testing.T) {
 		// Upload files to default userfile URI.
 		for _, file := range fileNames {
@@ -253,42 +247,42 @@ func TestUserFileList(t *testing.T) {
 			{
 				"list-all-in-default-using-star",
 				"*",
-				abs(fileNames),
+				fileNames,
 			},
 			{
 				"no-uri-list-all-in-default",
 				"",
-				abs(fileNames),
+				fileNames,
 			},
 			{
 				"no-glob-path-list-all-in-default",
 				defaultUserfileURLSchemeAndHost.String(),
-				abs(fileNames),
+				fileNames,
 			},
 			{
 				"no-host-list-all-in-default",
 				"userfile:///*/*/*.csv",
-				abs(fileNames),
+				fileNames,
 			},
 			{
 				"well-formed-userfile-uri",
 				defaultUserfileURLSchemeAndHost.String() + "/file/letters/*.csv",
-				abs(dataLetterFiles),
+				dataLetterFiles,
 			},
 			{
 				"only-glob",
 				"file/letters/*.csv",
-				abs(dataLetterFiles),
+				dataLetterFiles,
 			},
 			{
 				"list-data-num-csv",
 				"file/numbers/data[0-9].csv",
-				abs(dataNumberFiles),
+				dataNumberFiles,
 			},
 			{
 				"wildcard-bucket-and-filename",
 				"*/numbers/*.csv",
-				abs(dataNumberFiles),
+				dataNumberFiles,
 			},
 			{
 				"list-all-csv-skip-dir",
@@ -347,18 +341,6 @@ func TestUserFileDelete(t *testing.T) {
 		Host:   defaultQualifiedNamePrefix + security.RootUser,
 	}
 
-	abs := func(in []string) []string {
-		if in == nil {
-			return nil
-		}
-
-		out := make([]string, len(in))
-		for i := range in {
-			out[i] = defaultUserfileURLSchemeAndHost.String() + "/" + in[i]
-		}
-		return out
-	}
-
 	t.Run("DeleteFiles", func(t *testing.T) {
 		for _, tc := range []struct {
 			name               string
@@ -368,52 +350,73 @@ func TestUserFileDelete(t *testing.T) {
 			postDeleteList     []string
 		}{
 			{
-				"delete-all-in-default",
-				"*",
+				"delete-all",
+				"",
+				fileNames,
+				fileNames,
+				nil,
+			},
+			{
+				"delete-all-with-slash",
+				"/",
+				fileNames,
+				fileNames,
+				nil,
+			},
+			{
+				"delete-all-with-slash-prefix",
+				"/file",
+				fileNames,
+				fileNames,
+				nil,
+			},
+			{
+				"delete-all-with-prefix",
+				"file",
+				fileNames,
 				fileNames,
-				abs(fileNames),
 				nil,
 			},
 			{
 				"no-glob-path-delete-all-in-default",
 				defaultUserfileURLSchemeAndHost.String(),
 				fileNames,
-				abs(fileNames),
+				fileNames,
 				nil,
 			},
 			{
 				"no-host-delete-all-in-default",
 				"userfile:///*/*/*.*",
 				fileNames,
-				abs(fileNames),
+				fileNames,
 				nil,
 			},
 			{
 				"well-formed-userfile-uri",
 				defaultUserfileURLSchemeAndHost.String() + "/file/letters/*.csv",
 				fileNames,
-				abs(dataLetterFiles),
+				dataLetterFiles,
 				append(dataNumberFiles, unicodeFile...),
 			},
 			{
 				"delete-unicode-file",
 				defaultUserfileURLSchemeAndHost.String() + "/file/unicode/á.csv",
 				fileNames,
-				abs(unicodeFile),
+				unicodeFile,
 				append(dataLetterFiles, dataNumberFiles...),
 			},
 			{
 				"delete-data-num-csv",
 				"file/numbers/data[0-9].csv",
 				fileNames,
-				abs(dataNumberFiles),
+				dataNumberFiles,
 				append(dataLetterFiles, unicodeFile...),
 			},
 			{
 				"wildcard-bucket-and-filename",
 				"*/numbers/*.csv",
 				fileNames,
-				abs(dataNumberFiles),
+				dataNumberFiles,
 				append(dataLetterFiles, unicodeFile...),
 			},
 			{
@@ -455,16 +458,16 @@ func TestUserFileDelete(t *testing.T) {
 				}
 
 				// List files prior to deletion.
-				checkListedFiles(t, c, "", "", abs(tc.writeList))
+				checkListedFiles(t, c, "", "", tc.writeList)
 
 				// Delete files.
 				checkDeletedFiles(t, c, tc.URI, "", tc.expectedDeleteList)
 
 				// List files after deletion.
-				checkListedFiles(t, c, "", "", abs(tc.postDeleteList))
+				checkListedFiles(t, c, "", "", tc.postDeleteList)
 
 				// Cleanup all files for next test run.
-				_, err = c.RunWithCaptureArgs([]string{"userfile", "delete", "*"})
+				_, err = c.RunWithCaptureArgs([]string{"userfile", "delete", "/"})
 				require.NoError(t, err)
 			})
 		}
@@ -535,10 +538,10 @@ func TestUsernameUserfileInteraction(t *testing.T) {
 			uri := constructUserfileDestinationURI("", tc.name, user)
 			checkUserFileContent(ctx, t, c.ExecutorConfig(), user, uri, fileContent)
 
-			checkListedFiles(t, c, "", fmt.Sprintf("--url=%s", userURL.String()), []string{uri})
+			checkListedFiles(t, c, "", fmt.Sprintf("--url=%s", userURL.String()), []string{tc.name})
 
 			checkDeletedFiles(t, c, "", fmt.Sprintf("--url=%s", userURL.String()),
-				[]string{uri})
+				[]string{tc.name})
 		}
 	})
 }