From d580ec984dbe74bc4569124c2dc436569698b451 Mon Sep 17 00:00:00 2001 From: zveinn Date: Fri, 6 Oct 2023 12:45:10 +0000 Subject: [PATCH 1/8] refactored base validation in order to reduce HEAD calls on various cp and mv commands --- cmd/common-methods.go | 12 ++-- cmd/cp-main.go | 72 ++++++++++++--------- cmd/cp-url-syntax.go | 70 +------------------- cmd/cp-url.go | 145 ++++++++++++++++++++++++++++++------------ cmd/du-main.go | 4 +- cmd/mv-main.go | 2 +- cmd/od-main.go | 2 +- cmd/rm-main.go | 2 +- cmd/typed-errors.go | 28 ++++++++ functional-tests.sh | 55 ++++++++++++---- 10 files changed, 232 insertions(+), 160 deletions(-) diff --git a/cmd/common-methods.go b/cmd/common-methods.go index 41eb6ab38e..1f86ca5858 100644 --- a/cmd/common-methods.go +++ b/cmd/common-methods.go @@ -108,12 +108,12 @@ func getEncKeys(ctx *cli.Context) (map[string][]prefixSSEPair, *probe.Error) { // Check if the passed URL represents a folder. It may or may not exist yet. // If it exists, we can easily check if it is a folder, if it doesn't exist, // we can guess if the url is a folder from how it looks. -func isAliasURLDir(ctx context.Context, aliasURL string, keys map[string][]prefixSSEPair, timeRef time.Time) bool { +func isAliasURLDir(ctx context.Context, aliasURL string, keys map[string][]prefixSSEPair, timeRef time.Time) (bool, *ClientContent) { // If the target url exists, check if it is a directory // and return immediately. _, targetContent, err := url2Stat(ctx, aliasURL, "", false, keys, timeRef, false) if err == nil { - return targetContent.Type.IsDir() + return targetContent.Type.IsDir(), targetContent } _, expandedURL, _ := mustExpandAlias(aliasURL) @@ -121,7 +121,7 @@ func isAliasURLDir(ctx context.Context, aliasURL string, keys map[string][]prefi // Check if targetURL is an FS or S3 aliased url if expandedURL == aliasURL { // This is an FS url, check if the url has a separator at the end - return strings.HasSuffix(aliasURL, string(filepath.Separator)) + return strings.HasSuffix(aliasURL, string(filepath.Separator)), targetContent } // This is an S3 url, then: @@ -134,14 +134,14 @@ func isAliasURLDir(ctx context.Context, aliasURL string, keys map[string][]prefi switch len(fields) { // Nothing or alias format case 0, 1: - return false + return false, targetContent // alias/bucket format case 2: - return true + return true, targetContent } // default case.. // alias/bucket/prefix format - return strings.HasSuffix(pathURL, "/") + return strings.HasSuffix(pathURL, "/"), targetContent } // getSourceStreamMetadataFromURL gets a reader from URL. diff --git a/cmd/cp-main.go b/cmd/cp-main.go index 23f49b130c..983a6778e8 100644 --- a/cmd/cp-main.go +++ b/cmd/cp-main.go @@ -288,7 +288,7 @@ func doCopyFake(cpURLs URLs, pg Progress) URLs { } // doPrepareCopyURLs scans the source URL and prepares a list of objects for copying. -func doPrepareCopyURLs(ctx context.Context, session *sessionV8, cancelCopy context.CancelFunc) (totalBytes, totalObjects int64) { +func doPrepareCopyURLs(ctx context.Context, session *sessionV8, cancelCopy context.CancelFunc) (totalBytes, totalObjects int64, errSeen bool) { // Separate source and target. 'cp' can take only one target, // but any number of sources. sourceURLs := session.Header.CommandArgs[:len(session.Header.CommandArgs)-1] @@ -333,16 +333,10 @@ func doPrepareCopyURLs(ctx context.Context, session *sessionV8, cancelCopy conte done = true break } + if cpURLs.Error != nil { - // Print in new line and adjust to top so that we don't print over the ongoing scan bar - if !globalQuiet && !globalJSON { - console.Eraseline() - } - if strings.Contains(cpURLs.Error.ToGoError().Error(), " is a folder.") { - errorIf(cpURLs.Error.Trace(), "Folder cannot be copied. Please use `...` suffix.") - } else { - errorIf(cpURLs.Error.Trace(), "Unable to prepare URL for copying.") - } + printCopyURLsError(&cpURLs) + errSeen = true break } @@ -377,11 +371,30 @@ func doPrepareCopyURLs(ctx context.Context, session *sessionV8, cancelCopy conte return } +func printCopyURLsError(cpURLs *URLs) { + + // Print in new line and adjust to top so that we + // don't print over the ongoing scan bar + if !globalQuiet && !globalJSON { + console.Eraseline() + } + + if strings.Contains(cpURLs.Error.ToGoError().Error(), + " is a folder.") { + errorIf(cpURLs.Error.Trace(), + "Folder cannot be copied. Please use `...` suffix.") + } else { + errorIf(cpURLs.Error.Trace(), + "Unable to prepare URL for copying.") + } +} + func doCopySession(ctx context.Context, cancelCopy context.CancelFunc, cli *cli.Context, session *sessionV8, encKeyDB map[string][]prefixSSEPair, isMvCmd bool) error { var isCopied func(string) bool var totalObjects, totalBytes int64 cpURLsCh := make(chan URLs, 10000) + errSeen := false // Store a progress bar or an accounter var pg ProgressReader @@ -405,7 +418,7 @@ func doCopySession(ctx context.Context, cancelCopy context.CancelFunc, cli *cli. isCopied = isLastFactory(session.Header.LastCopied) if !session.HasData() { - totalBytes, totalObjects = doPrepareCopyURLs(ctx, session, cancelCopy) + totalBytes, totalObjects, errSeen = doPrepareCopyURLs(ctx, session, cancelCopy) } else { totalBytes, totalObjects = session.Header.TotalBytes, session.Header.TotalObjects } @@ -431,6 +444,7 @@ func doCopySession(ctx context.Context, cancelCopy context.CancelFunc, cli *cli. cpURLsCh <- cpURLs } }() + } else { // Access recursive flag inside the session header. isRecursive := cli.Bool("recursive") @@ -452,23 +466,14 @@ func doCopySession(ctx context.Context, cancelCopy context.CancelFunc, cli *cli. versionID: versionID, isZip: cli.Bool("zip"), } + for cpURLs := range prepareCopyURLs(ctx, opts) { if cpURLs.Error != nil { - // Print in new line and adjust to top so that we - // don't print over the ongoing scan bar - if !globalQuiet && !globalJSON { - console.Eraseline() - } - if strings.Contains(cpURLs.Error.ToGoError().Error(), - " is a folder.") { - errorIf(cpURLs.Error.Trace(), - "Folder cannot be copied. Please use `...` suffix.") - } else { - errorIf(cpURLs.Error.Trace(), - "Unable to start copying.") - } + errSeen = true + printCopyURLsError(&cpURLs) break } + totalBytes += cpURLs.SourceContent.Size pg.SetTotal(totalBytes) totalObjects++ @@ -570,7 +575,6 @@ func doCopySession(ctx context.Context, cancelCopy context.CancelFunc, cli *cli. }() var retErr error - errSeen := false cpAllFilesErr := true loop: @@ -639,14 +643,24 @@ loop: } if progressReader, ok := pg.(*progressBar); ok { - if (errSeen && totalObjects == 1) || (cpAllFilesErr && totalObjects > 1) { - console.Eraseline() + if errSeen || (cpAllFilesErr && totalObjects > 0) { + // We only erase a line if we are displaying a progress bar + if !globalQuiet && !globalJSON { + console.Eraseline() + } } else if progressReader.ProgressBar.Get() > 0 { progressReader.ProgressBar.Finish() } } else { if accntReader, ok := pg.(*accounter); ok { - printMsg(accntReader.Stat()) + if errSeen || (cpAllFilesErr && totalObjects > 0) { + // We only erase a line if we are displaying a progress bar + if !globalQuiet && !globalJSON { + console.Eraseline() + } + } else { + printMsg(accntReader.Stat()) + } } } @@ -670,7 +684,7 @@ func mainCopy(cliCtx *cli.Context) error { } // check 'copy' cli arguments. - checkCopySyntax(ctx, cliCtx, encKeyDB, false) + checkCopySyntax(ctx, cliCtx, encKeyDB) // Additional command specific theme customization. console.SetColor("Copy", color.New(color.FgGreen, color.Bold)) diff --git a/cmd/cp-url-syntax.go b/cmd/cp-url-syntax.go index 85b661238a..db9de0fe08 100644 --- a/cmd/cp-url-syntax.go +++ b/cmd/cp-url-syntax.go @@ -24,15 +24,10 @@ import ( "time" "github.com/minio/cli" - "github.com/minio/mc/pkg/probe" - "github.com/minio/pkg/v2/console" ) -func checkCopySyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[string][]prefixSSEPair, isMvCmd bool) { +func checkCopySyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[string][]prefixSSEPair) { if len(cliCtx.Args()) < 2 { - if isMvCmd { - showCommandHelpAndExit(cliCtx, 1) // last argument is exit code. - } showCommandHelpAndExit(cliCtx, 1) // last argument is exit code. } @@ -44,9 +39,7 @@ func checkCopySyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[stri srcURLs := URLs[:len(URLs)-1] tgtURL := URLs[len(URLs)-1] - isRecursive := cliCtx.Bool("recursive") isZip := cliCtx.Bool("zip") - timeRef := parseRewindFlag(cliCtx.String("rewind")) versionID := cliCtx.String("version-id") if versionID != "" && len(srcURLs) > 1 { @@ -57,24 +50,6 @@ func checkCopySyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[stri fatalIf(errDummy().Trace(cliCtx.Args()...), "--zip and --rewind cannot be used together") } - // Verify if source(s) exists. - for _, srcURL := range srcURLs { - var err *probe.Error - if !isRecursive { - _, _, err = url2Stat(ctx, srcURL, versionID, false, encKeyDB, timeRef, isZip) - } else { - _, _, err = firstURL2Stat(ctx, srcURL, timeRef, isZip) - } - if err != nil { - msg := "Unable to validate source `" + srcURL + "`" - if versionID != "" { - msg += " (" + versionID + ")" - } - msg += ": " + err.ToGoError().Error() - console.Fatalln(msg) - } - } - // Check if bucket name is passed for URL type arguments. url := newClientURL(tgtURL) if url.Host != "" { @@ -91,49 +66,6 @@ func checkCopySyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[stri fatalIf(errInvalidArgument().Trace(), fmt.Sprintf("Both object retention flags `--%s` and `--%s` are required.\n", rdFlag, rmFlag)) } - operation := "copy" - if isMvCmd { - operation = "move" - } - - // Guess CopyURLsType based on source and target URLs. - opts := prepareCopyURLsOpts{ - sourceURLs: srcURLs, - targetURL: tgtURL, - isRecursive: isRecursive, - encKeyDB: encKeyDB, - olderThan: "", - newerThan: "", - timeRef: timeRef, - versionID: versionID, - isZip: isZip, - } - copyURLsType, _, err := guessCopyURLType(ctx, opts) - if err != nil { - fatalIf(errInvalidArgument().Trace(), "Unable to guess the type of "+operation+" operation.") - } - - switch copyURLsType { - case copyURLsTypeA: // File -> File. - // Check source. - if len(srcURLs) != 1 { - fatalIf(errInvalidArgument().Trace(), "Invalid number of source arguments.") - } - checkCopySyntaxTypeA(ctx, srcURLs[0], versionID, encKeyDB, isZip, timeRef) - case copyURLsTypeB: // File -> Folder. - // Check source. - if len(srcURLs) != 1 { - fatalIf(errInvalidArgument().Trace(), "Invalid number of source arguments.") - } - checkCopySyntaxTypeB(ctx, srcURLs[0], versionID, tgtURL, encKeyDB, isZip, timeRef) - case copyURLsTypeC: // Folder... -> Folder. - checkCopySyntaxTypeC(ctx, srcURLs, tgtURL, isRecursive, isZip, encKeyDB, isMvCmd, timeRef) - case copyURLsTypeD: // File1...FileN -> Folder. - checkCopySyntaxTypeD(ctx, tgtURL, encKeyDB, timeRef) - default: - fatalIf(errInvalidArgument().Trace(), "Unable to guess the type of "+operation+" operation.") - } - // Preserve functionality not supported for windows if cliCtx.Bool("preserve") && runtime.GOOS == "windows" { fatalIf(errInvalidArgument().Trace(), "Permissions are not preserved on windows platform.") diff --git a/cmd/cp-url.go b/cmd/cp-url.go index b9079a5451..3c32dbb805 100644 --- a/cmd/cp-url.go +++ b/cmd/cp-url.go @@ -53,7 +53,7 @@ const ( // guessCopyURLType guesses the type of clientURL. This approach all allows prepareURL // functions to accurately report failure causes. -func guessCopyURLType(ctx context.Context, o prepareCopyURLsOpts) (copyURLsType, string, *probe.Error) { +func guessCopyURLType(ctx context.Context, o prepareCopyURLsOpts) (copyURLsType, string, *ClientContent, *ClientContent, *probe.Error) { if len(o.sourceURLs) == 1 { // 1 Source, 1 Target var err *probe.Error var sourceContent *ClientContent @@ -64,44 +64,50 @@ func guessCopyURLType(ctx context.Context, o prepareCopyURLsOpts) (copyURLsType, _, sourceContent, err = firstURL2Stat(ctx, sourceURL, o.timeRef, o.isZip) } if err != nil { - return copyURLsTypeInvalid, "", err + return copyURLsTypeInvalid, "", sourceContent, nil, err } // If recursion is ON, it is type C. // If source is a folder, it is Type C. if sourceContent.Type.IsDir() || o.isRecursive { - return copyURLsTypeC, "", nil + return copyURLsTypeC, "", sourceContent, nil, nil } // If target is a folder, it is Type B. - if isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) { - return copyURLsTypeB, sourceContent.VersionID, nil + isDir, targetContent := isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) + if isDir { + return copyURLsTypeB, sourceContent.VersionID, sourceContent, targetContent, nil } // else Type A. - return copyURLsTypeA, sourceContent.VersionID, nil + return copyURLsTypeA, sourceContent.VersionID, sourceContent, targetContent, nil } // Multiple source args and target is a folder. It is Type D. - if isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) { - return copyURLsTypeD, "", nil + isDir, targetContent := isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) + if isDir { + return copyURLsTypeD, "", nil, targetContent, nil } - return copyURLsTypeInvalid, "", errInvalidArgument().Trace() + return copyURLsTypeInvalid, "", nil, nil, errInvalidArgument().Trace() } // SINGLE SOURCE - Type A: copy(f, f) -> copy(f, f) // prepareCopyURLsTypeA - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeA(ctx context.Context, sourceURL, sourceVersion, targetURL string, encKeyDB map[string][]prefixSSEPair, isZip bool) URLs { +func prepareCopyURLsTypeA(ctx context.Context, sourceContent *ClientContent, sourceURL, sourceVersion, targetURL string, encKeyDB map[string][]prefixSSEPair, isZip bool) URLs { // Extract alias before fiddling with the clientURL. sourceAlias, _, _ := mustExpandAlias(sourceURL) // Find alias and expanded clientURL. targetAlias, targetURL, _ := mustExpandAlias(targetURL) - _, sourceContent, err := url2Stat(ctx, sourceURL, sourceVersion, false, encKeyDB, time.Time{}, isZip) - if err != nil { - // Source does not exist or insufficient privileges. - return URLs{Error: err.Trace(sourceURL)} + var err *probe.Error + if sourceContent == nil { + _, sourceContent, err = url2Stat(ctx, sourceURL, sourceVersion, false, encKeyDB, time.Time{}, isZip) + if err != nil { + // Source does not exist or insufficient privileges. + return URLs{Error: err.Trace(sourceURL)} + } } + if !sourceContent.Type.IsRegular() { // Source is not a regular file return URLs{Error: errInvalidSource(sourceURL).Trace(sourceURL)} @@ -124,16 +130,19 @@ func makeCopyContentTypeA(sourceAlias string, sourceContent *ClientContent, targ // SINGLE SOURCE - Type B: copy(f, d) -> copy(f, d/f) -> A // prepareCopyURLsTypeB - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeB(ctx context.Context, sourceURL, sourceVersion, targetURL string, encKeyDB map[string][]prefixSSEPair, isZip bool) URLs { +func prepareCopyURLsTypeB(ctx context.Context, sourceContent, targetContent *ClientContent, sourceURL, sourceVersion, targetURL string, encKeyDB map[string][]prefixSSEPair, isZip bool) URLs { // Extract alias before fiddling with the clientURL. sourceAlias, _, _ := mustExpandAlias(sourceURL) // Find alias and expanded clientURL. targetAlias, targetURL, _ := mustExpandAlias(targetURL) - _, sourceContent, err := url2Stat(ctx, sourceURL, sourceVersion, false, encKeyDB, time.Time{}, isZip) - if err != nil { - // Source does not exist or insufficient privileges. - return URLs{Error: err.Trace(sourceURL)} + var err *probe.Error + if sourceContent == nil { + _, sourceContent, err = url2Stat(ctx, sourceURL, sourceVersion, false, encKeyDB, time.Time{}, isZip) + if err != nil { + // Source does not exist or insufficient privileges. + return URLs{Error: err.Trace(sourceURL)} + } } if !sourceContent.Type.IsRegular() { @@ -144,6 +153,15 @@ func prepareCopyURLsTypeB(ctx context.Context, sourceURL, sourceVersion, targetU return URLs{Error: errInvalidSource(sourceURL).Trace(sourceURL)} } + if targetContent == nil { + _, targetContent, err = url2Stat(ctx, targetURL, "", false, encKeyDB, time.Time{}, false) + if err == nil { + if !targetContent.Type.IsDir() { + return URLs{Error: errInvalidTarget(targetURL).Trace(targetURL)} + } + } + } + // All OK.. We can proceed. Type B: source is a file, target is a folder and exists. return makeCopyContentTypeB(sourceAlias, sourceContent, targetAlias, targetURL) } @@ -158,20 +176,62 @@ func makeCopyContentTypeB(sourceAlias string, sourceContent *ClientContent, targ // SINGLE SOURCE - Type C: copy(d1..., d2) -> []copy(d1/f, d1/d2/f) -> []A // prepareCopyRecursiveURLTypeC - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeC(ctx context.Context, sourceURL, targetURL string, isRecursive, isZip bool, timeRef time.Time) <-chan URLs { +func prepareCopyURLsTypeC(ctx context.Context, sourceContent, targetContent *ClientContent, sourceURL, targetURL string, encKeyDB map[string][]prefixSSEPair, isRecursive, isZip bool, timeRef time.Time) <-chan URLs { // Extract alias before fiddling with the clientURL. sourceAlias, _, _ := mustExpandAlias(sourceURL) // Find alias and expanded clientURL. targetAlias, targetURL, _ := mustExpandAlias(targetURL) - copyURLsCh := make(chan URLs) - go func(sourceURL, targetURL string, copyURLsCh chan URLs) { - defer close(copyURLsCh) - sourceClient, err := newClient(sourceURL) + copyURLsCh := make(chan URLs, 1) + + returnErrorAndCloseChannel := func(err *probe.Error) chan URLs { + copyURLsCh <- URLs{Error: err} + close(copyURLsCh) + return copyURLsCh + } + + c, err := newClient(sourceURL) + if err != nil { + return returnErrorAndCloseChannel(err.Trace(sourceURL)) + } + + if targetContent == nil { + _, targetContent, err = url2Stat(ctx, targetURL, "", false, encKeyDB, time.Time{}, isZip) if err != nil { - // Source initialization failed. - copyURLsCh <- URLs{Error: err.Trace(sourceURL)} - return + return returnErrorAndCloseChannel(err.Trace(targetURL)) } + } + + if !targetContent.Type.IsDir() { + return returnErrorAndCloseChannel(errTargetIsNotDir(targetURL).Trace(targetURL)) + } + + if sourceContent == nil { + _, sourceContent, err = url2Stat(ctx, sourceURL, "", false, encKeyDB, time.Time{}, isZip) + if err != nil { + return returnErrorAndCloseChannel(err.Trace(sourceURL)) + } + } + + if sourceContent.Type.IsDir() { + // Require --recursive flag if we are copying a directory + if !isRecursive { + return returnErrorAndCloseChannel(errRequiresRecursive(sourceURL).Trace(sourceURL)) + } + + // Check if we are going to copy a directory into itself + if isURLContains(sourceURL, targetURL, string(c.GetURL().Separator)) { + return returnErrorAndCloseChannel(errCopyIntoSelf(sourceURL).Trace(targetURL)) + } + } + + // if !isRecursive { + // copyURLsCh <- makeCopyContentTypeC(sourceAlias, c.GetURL(), sourceContent, targetAlias, targetURL) + // close(copyURLsCh) + // return copyURLsCh + // } + + go func(sourceClient Client, sourceURL, targetURL string, copyURLsCh chan URLs) { + defer close(copyURLsCh) for sourceContent := range sourceClient.List(ctx, ListOptions{Recursive: isRecursive, TimeRef: timeRef, ShowDir: DirNone, ListZip: isZip}) { if sourceContent.Err != nil { @@ -188,7 +248,9 @@ func prepareCopyURLsTypeC(ctx context.Context, sourceURL, targetURL string, isRe // All OK.. We can proceed. Type B: source is a file, target is a folder and exists. copyURLsCh <- makeCopyContentTypeC(sourceAlias, sourceClient.GetURL(), sourceContent, targetAlias, targetURL) } - }(sourceURL, targetURL, copyURLsCh) + + }(c, sourceURL, targetURL, copyURLsCh) + return copyURLsCh } @@ -207,16 +269,18 @@ func makeCopyContentTypeC(sourceAlias string, sourceURL ClientURL, sourceContent // MULTI-SOURCE - Type D: copy([](f|d...), d) -> []B // prepareCopyURLsTypeE - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeD(ctx context.Context, sourceURLs []string, targetURL string, isRecursive bool, timeRef time.Time) <-chan URLs { - copyURLsCh := make(chan URLs) - go func(sourceURLs []string, targetURL string, copyURLsCh chan URLs) { +func prepareCopyURLsTypeD(ctx context.Context, sourceContent *ClientContent, targetContent *ClientContent, sourceURLs []string, targetURL string, encKeyDB map[string][]prefixSSEPair, isRecursive bool, timeRef time.Time, isZip bool) <-chan URLs { + copyURLsCh := make(chan URLs, 1) + + go func(sourceURLs []string, sourceContent, targetContent *ClientContent, targetURL string, copyURLsCh chan URLs) { defer close(copyURLsCh) for _, sourceURL := range sourceURLs { - for cpURLs := range prepareCopyURLsTypeC(ctx, sourceURL, targetURL, isRecursive, false, timeRef) { + for cpURLs := range prepareCopyURLsTypeC(ctx, nil, targetContent, sourceURL, targetURL, encKeyDB, isRecursive, isZip, timeRef) { copyURLsCh <- cpURLs } } - }(sourceURLs, targetURL, copyURLsCh) + }(sourceURLs, sourceContent, targetContent, targetURL, copyURLsCh) + return copyURLsCh } @@ -236,20 +300,23 @@ func prepareCopyURLs(ctx context.Context, o prepareCopyURLsOpts) chan URLs { copyURLsCh := make(chan URLs) go func(o prepareCopyURLsOpts) { defer close(copyURLsCh) - cpType, cpVersion, err := guessCopyURLType(ctx, o) - fatalIf(err.Trace(), "Unable to guess the type of copy operation.") + cpType, cpVersion, sourceContent, targetContent, err := guessCopyURLType(ctx, o) + if err != nil { + copyURLsCh <- URLs{Error: errUnableToGuess().Trace(o.sourceURLs...)} + return + } switch cpType { case copyURLsTypeA: - copyURLsCh <- prepareCopyURLsTypeA(ctx, o.sourceURLs[0], cpVersion, o.targetURL, o.encKeyDB, o.isZip) + copyURLsCh <- prepareCopyURLsTypeA(ctx, sourceContent, o.sourceURLs[0], cpVersion, o.targetURL, o.encKeyDB, o.isZip) case copyURLsTypeB: - copyURLsCh <- prepareCopyURLsTypeB(ctx, o.sourceURLs[0], cpVersion, o.targetURL, o.encKeyDB, o.isZip) + copyURLsCh <- prepareCopyURLsTypeB(ctx, sourceContent, targetContent, o.sourceURLs[0], cpVersion, o.targetURL, o.encKeyDB, o.isZip) case copyURLsTypeC: - for cURLs := range prepareCopyURLsTypeC(ctx, o.sourceURLs[0], o.targetURL, o.isRecursive, o.isZip, o.timeRef) { + for cURLs := range prepareCopyURLsTypeC(ctx, sourceContent, targetContent, o.sourceURLs[0], o.targetURL, o.encKeyDB, o.isRecursive, o.isZip, o.timeRef) { copyURLsCh <- cURLs } case copyURLsTypeD: - for cURLs := range prepareCopyURLsTypeD(ctx, o.sourceURLs, o.targetURL, o.isRecursive, o.timeRef) { + for cURLs := range prepareCopyURLsTypeD(ctx, sourceContent, targetContent, o.sourceURLs, o.targetURL, o.encKeyDB, o.isRecursive, o.timeRef, o.isZip) { copyURLsCh <- cURLs } default: diff --git a/cmd/du-main.go b/cmd/du-main.go index 5ced3982e3..df7e0a4358 100644 --- a/cmd/du-main.go +++ b/cmd/du-main.go @@ -243,8 +243,10 @@ func mainDu(cliCtx *cli.Context) error { timeRef := parseRewindFlag(cliCtx.String("rewind")) var duErr error + var isDir bool for _, urlStr := range cliCtx.Args() { - if !isAliasURLDir(ctx, urlStr, nil, time.Time{}) { + isDir, _ = isAliasURLDir(ctx, urlStr, nil, time.Time{}) + if !isDir { fatalIf(errInvalidArgument().Trace(urlStr), fmt.Sprintf("Source `%s` is not a folder. Only folders are supported by 'du' command.", urlStr)) } diff --git a/cmd/mv-main.go b/cmd/mv-main.go index 2420865ecb..d5e238ae84 100644 --- a/cmd/mv-main.go +++ b/cmd/mv-main.go @@ -227,7 +227,7 @@ func mainMove(cliCtx *cli.Context) error { } // check 'copy' cli arguments. - checkCopySyntax(ctx, cliCtx, encKeyDB, true) + checkCopySyntax(ctx, cliCtx, encKeyDB) if cliCtx.NArg() == 2 { args := cliCtx.Args() diff --git a/cmd/od-main.go b/cmd/od-main.go index 15b1fa13ff..fd012cc38f 100644 --- a/cmd/od-main.go +++ b/cmd/od-main.go @@ -105,7 +105,7 @@ func getOdUrls(ctx context.Context, args argKVS) (odURLs URLs, e error) { sourceURLs: []string{inFile}, targetURL: outFile, } - odType, _, err := guessCopyURLType(ctx, opts) + odType, _, _, _, err := guessCopyURLType(ctx, opts) fatalIf(err, "Unable to guess copy URL type") // Get content of inFile, set up URLs. diff --git a/cmd/rm-main.go b/cmd/rm-main.go index b1baa794f3..0a2f4589f2 100644 --- a/cmd/rm-main.go +++ b/cmd/rm-main.go @@ -253,7 +253,7 @@ func checkRmSyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[string // Note: UNC path using / works properly in go 1.9.2 even though it breaks the UNC specification. url = filepath.ToSlash(filepath.Clean(url)) // namespace removal applies only for non FS. So filter out if passed url represents a directory - dir := isAliasURLDir(ctx, url, encKeyDB, time.Time{}) + dir, _ := isAliasURLDir(ctx, url, encKeyDB, time.Time{}) if dir { _, path := url2Alias(url) isNamespaceRemoval = (path == "") diff --git a/cmd/typed-errors.go b/cmd/typed-errors.go index 12af6aefb6..cd00ec8b10 100644 --- a/cmd/typed-errors.go +++ b/cmd/typed-errors.go @@ -39,6 +39,13 @@ var errInvalidArgument = func() *probe.Error { return probe.NewError(invalidArgumentErr(errors.New(msg))).Untrace() } +type unableToGuessErr error + +var errUnableToGuess = func() *probe.Error { + msg := "Unable to guess the type of copy operation." + return probe.NewError(unableToGuessErr(errors.New(msg))) +} + type unrecognizedDiffTypeErr error var errUnrecognizedDiffType = func(diff differType) *probe.Error { @@ -97,6 +104,20 @@ var errInvalidTarget = func(URL string) *probe.Error { return probe.NewError(invalidTargetErr(errors.New(msg))).Untrace() } +type requiresRecuriveErr error + +var errRequiresRecursive = func(URL string) *probe.Error { + msg := "To copy or move '" + URL + "' the --recursive flag is required." + return probe.NewError(requiresRecuriveErr(errors.New(msg))).Untrace() +} + +type CopyIntoSelfErr error + +var errCopyIntoSelf = func(URL string) *probe.Error { + msg := "Copying or moving '" + URL + "' into itself is not allowed." + return probe.NewError(CopyIntoSelfErr(errors.New(msg))).Untrace() +} + type targetNotFoundErr error var errTargetNotFound = func(URL string) *probe.Error { @@ -113,6 +134,13 @@ var errOverWriteNotAllowed = func(URL string) *probe.Error { return probe.NewError(overwriteNotAllowedErr{errors.New(msg)}) } +type targetIsNotDirErr error + +var errTargetIsNotDir = func(URL string) *probe.Error { + msg := "Target `" + URL + "` is not a folder." + return probe.NewError(targetIsNotDirErr(errors.New(msg))).Untrace() +} + type sourceIsDirErr error var errSourceIsDir = func(URL string) *probe.Error { diff --git a/functional-tests.sh b/functional-tests.sh index c18bd7886c..b7d19893fe 100755 --- a/functional-tests.sh +++ b/functional-tests.sh @@ -983,6 +983,11 @@ function test_admin_users() # create a user username=foo password=foobar12345 + test_alias="aliasx" + + # Remove user incase the user was created and not removed due to a failed test + mc_cmd admin user remove "$SERVER_ALIAS" "$username" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin user add "$SERVER_ALIAS" "$username" "$password" # check that user appears in the user list @@ -993,49 +998,61 @@ function test_admin_users() # setup temporary alias to make requests as the created user. scheme="https" if [ "$ENABLE_HTTPS" != "1" ]; then - scheme="http" + scheme="http" fi object1_name="mc-test-object-$RANDOM" object2_name="mc-test-object-$RANDOM" - export MC_HOST_foo="${scheme}://${username}:${password}@${SERVER_ENDPOINT}" + + # Adding an alias for the $test_alias + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd alias set $test_alias "${scheme}://${SERVER_ENDPOINT}" ${username} ${password} + + # check that alias appears in the alias list + "${MC_CMD[@]}" --json alias list | jq -r '.alias' | grep --quiet "^${test_alias}$" + rv=$? + assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure ${rv} "alias ${test_alias} did NOT appear in the list of aliases returned by server" # check that the user can write objects with readwrite policy assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin policy attach "$SERVER_ALIAS" readwrite --user="${username}" - assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cp "$FILE_1_MB" "foo/${BUCKET_NAME}/${object1_name}" + + # Validate that the correct policy has been added to the user + "${MC_CMD[@]}" --json admin user list "${SERVER_ALIAS}" | jq -r '.policyName' | grep --quiet "^readwrite$" + rv=$? + assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure ${rv} "user ${username} did NOT have the readwrite policy attached" + + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd mb "${test_alias}/${BUCKET_NAME}" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cp "$FILE_1_MB" "${test_alias}/${BUCKET_NAME}/${object1_name}" # check that the user cannot write objects with readonly policy assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin policy detach "$SERVER_ALIAS" readwrite --user="${username}" assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin policy attach "$SERVER_ALIAS" readonly --user="${username}" - assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd cp "$FILE_1_MB" "foo/${BUCKET_NAME}/${object2_name}" + assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd cp "$FILE_1_MB" "${test_alias}/${BUCKET_NAME}/${object2_name}" # check that the user can read with readonly policy - assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cat "foo/${BUCKET_NAME}/${object1_name}" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cat "${test_alias}/${BUCKET_NAME}/${object1_name}" # check that user can delete with readwrite policy assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin policy attach "$SERVER_ALIAS" readwrite --user="${username}" - assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd rm "foo/${BUCKET_NAME}/${object1_name}" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd rm "${test_alias}/${BUCKET_NAME}/${object1_name}" # check that user cannot perform admin actions with readwrite policy - assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd admin info "foo" + assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd admin info $test_alias # create object1_name for subsequent tests. - assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cp "$FILE_1_MB" "foo/${BUCKET_NAME}/${object1_name}" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cp "$FILE_1_MB" "${test_alias}/${BUCKET_NAME}/${object1_name}" # check that user can be disabled assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin user disable "$SERVER_ALIAS" "$username" # check that disabled cannot perform any action - assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd cat "foo/${BUCKET_NAME}/${object1_name}" + assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd cat "${test_alias}/${BUCKET_NAME}/${object1_name}" # check that user can be enabled and can then perform an allowed action assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin user enable "$SERVER_ALIAS" "$username" - assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cat "foo/${BUCKET_NAME}/${object1_name}" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cat "${test_alias}/${BUCKET_NAME}/${object1_name}" # check that user can be removed, and then is no longer available assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin user remove "$SERVER_ALIAS" "$username" - assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd cat "foo/${BUCKET_NAME}/${object1_name}" - - unset MC_HOST_foo + assert_failure "$start_time" "${FUNCNAME[0]}" mc_cmd cat "${test_alias}/${BUCKET_NAME}/${object1_name}" log_success "$start_time" "${FUNCNAME[0]}" } @@ -1162,8 +1179,20 @@ function __init__() set +e } +function validate_dependencies() { + jqVersion=$(jq --version) + if [[ $jqVersion == *"jq"* ]]; then + echo "Dependency validation complete" + else + echo "jq is missing, please install: 'sudo apt install jq'" + exit 1 + fi +} + function main() { + validate_dependencies + ( run_test ) rv=$? From 244f2d7c5df8ff4a0d0747471a618e8a9f1c37a9 Mon Sep 17 00:00:00 2001 From: zveinn Date: Tue, 10 Oct 2023 08:11:58 +0000 Subject: [PATCH 2/8] Removing un-used methods --- cmd/cp-url-syntax.go | 79 -------------------------------------------- 1 file changed, 79 deletions(-) diff --git a/cmd/cp-url-syntax.go b/cmd/cp-url-syntax.go index db9de0fe08..b14d6d0011 100644 --- a/cmd/cp-url-syntax.go +++ b/cmd/cp-url-syntax.go @@ -21,7 +21,6 @@ import ( "context" "fmt" "runtime" - "time" "github.com/minio/cli" ) @@ -71,81 +70,3 @@ func checkCopySyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[stri fatalIf(errInvalidArgument().Trace(), "Permissions are not preserved on windows platform.") } } - -// checkCopySyntaxTypeA verifies if the source and target are valid file arguments. -func checkCopySyntaxTypeA(ctx context.Context, srcURL, versionID string, keys map[string][]prefixSSEPair, isZip bool, timeRef time.Time) { - _, srcContent, err := url2Stat(ctx, srcURL, versionID, false, keys, timeRef, isZip) - fatalIf(err.Trace(srcURL), "Unable to stat source `"+srcURL+"`.") - - if !srcContent.Type.IsRegular() { - fatalIf(errInvalidArgument().Trace(), "Source `"+srcURL+"` is not a file.") - } -} - -// checkCopySyntaxTypeB verifies if the source is a valid file and target is a valid folder. -func checkCopySyntaxTypeB(ctx context.Context, srcURL, versionID, tgtURL string, keys map[string][]prefixSSEPair, isZip bool, timeRef time.Time) { - _, srcContent, err := url2Stat(ctx, srcURL, versionID, false, keys, timeRef, isZip) - fatalIf(err.Trace(srcURL), "Unable to stat source `"+srcURL+"`.") - - if !srcContent.Type.IsRegular() { - fatalIf(errInvalidArgument().Trace(srcURL), "Source `"+srcURL+"` is not a file.") - } - - // Check target. - if _, tgtContent, err := url2Stat(ctx, tgtURL, "", false, keys, timeRef, false); err == nil { - if !tgtContent.Type.IsDir() { - fatalIf(errInvalidArgument().Trace(tgtURL), "Target `"+tgtURL+"` is not a folder.") - } - } -} - -// checkCopySyntaxTypeC verifies if the source is a valid recursive dir and target is a valid folder. -func checkCopySyntaxTypeC(ctx context.Context, srcURLs []string, tgtURL string, isRecursive, isZip bool, keys map[string][]prefixSSEPair, isMvCmd bool, timeRef time.Time) { - // Check source. - if len(srcURLs) != 1 { - fatalIf(errInvalidArgument().Trace(), "Invalid number of source arguments.") - } - - // Check target. - if _, tgtContent, err := url2Stat(ctx, tgtURL, "", false, keys, timeRef, false); err == nil { - if !tgtContent.Type.IsDir() { - fatalIf(errInvalidArgument().Trace(tgtURL), "Target `"+tgtURL+"` is not a folder.") - } - } - - for _, srcURL := range srcURLs { - c, srcContent, err := url2Stat(ctx, srcURL, "", false, keys, timeRef, isZip) - fatalIf(err.Trace(srcURL), "Unable to stat source `"+srcURL+"`.") - - if srcContent.Type.IsDir() { - // Require --recursive flag if we are copying a directory - if !isRecursive { - operation := "copy" - if isMvCmd { - operation = "move" - } - fatalIf(errInvalidArgument().Trace(srcURL), fmt.Sprintf("To %v a folder requires --recursive flag.", operation)) - } - - // Check if we are going to copy a directory into itself - if isURLContains(srcURL, tgtURL, string(c.GetURL().Separator)) { - operation := "Copying" - if isMvCmd { - operation = "Moving" - } - fatalIf(errInvalidArgument().Trace(), fmt.Sprintf("%v a folder into itself is not allowed.", operation)) - } - } - } -} - -// checkCopySyntaxTypeD verifies if the source is a valid list of files and target is a valid folder. -func checkCopySyntaxTypeD(ctx context.Context, tgtURL string, keys map[string][]prefixSSEPair, timeRef time.Time) { - // Source can be anything: file, dir, dir... - // Check target if it is a dir - if _, tgtContent, err := url2Stat(ctx, tgtURL, "", false, keys, timeRef, false); err == nil { - if !tgtContent.Type.IsDir() { - fatalIf(errInvalidArgument().Trace(tgtURL), "Target `"+tgtURL+"` is not a folder.") - } - } -} From 8a3a902a19e7db3786830544c7610acbb4a7a2a8 Mon Sep 17 00:00:00 2001 From: zveinn Date: Tue, 10 Oct 2023 09:18:57 +0000 Subject: [PATCH 3/8] Updates after gofumpt + golangci-lint --- cmd/cp-main.go | 3 +-- cmd/cp-url-syntax.go | 3 +-- cmd/cp-url.go | 1 - cmd/mv-main.go | 2 +- cmd/typed-errors.go | 4 ++-- 5 files changed, 5 insertions(+), 8 deletions(-) diff --git a/cmd/cp-main.go b/cmd/cp-main.go index 983a6778e8..2f61120bf6 100644 --- a/cmd/cp-main.go +++ b/cmd/cp-main.go @@ -372,7 +372,6 @@ func doPrepareCopyURLs(ctx context.Context, session *sessionV8, cancelCopy conte } func printCopyURLsError(cpURLs *URLs) { - // Print in new line and adjust to top so that we // don't print over the ongoing scan bar if !globalQuiet && !globalJSON { @@ -684,7 +683,7 @@ func mainCopy(cliCtx *cli.Context) error { } // check 'copy' cli arguments. - checkCopySyntax(ctx, cliCtx, encKeyDB) + checkCopySyntax(cliCtx) // Additional command specific theme customization. console.SetColor("Copy", color.New(color.FgGreen, color.Bold)) diff --git a/cmd/cp-url-syntax.go b/cmd/cp-url-syntax.go index b14d6d0011..4c3865c066 100644 --- a/cmd/cp-url-syntax.go +++ b/cmd/cp-url-syntax.go @@ -18,14 +18,13 @@ package cmd import ( - "context" "fmt" "runtime" "github.com/minio/cli" ) -func checkCopySyntax(ctx context.Context, cliCtx *cli.Context, encKeyDB map[string][]prefixSSEPair) { +func checkCopySyntax(cliCtx *cli.Context) { if len(cliCtx.Args()) < 2 { showCommandHelpAndExit(cliCtx, 1) // last argument is exit code. } diff --git a/cmd/cp-url.go b/cmd/cp-url.go index 3c32dbb805..415f23dab0 100644 --- a/cmd/cp-url.go +++ b/cmd/cp-url.go @@ -248,7 +248,6 @@ func prepareCopyURLsTypeC(ctx context.Context, sourceContent, targetContent *Cli // All OK.. We can proceed. Type B: source is a file, target is a folder and exists. copyURLsCh <- makeCopyContentTypeC(sourceAlias, sourceClient.GetURL(), sourceContent, targetAlias, targetURL) } - }(c, sourceURL, targetURL, copyURLsCh) return copyURLsCh diff --git a/cmd/mv-main.go b/cmd/mv-main.go index d5e238ae84..92ddd74c60 100644 --- a/cmd/mv-main.go +++ b/cmd/mv-main.go @@ -227,7 +227,7 @@ func mainMove(cliCtx *cli.Context) error { } // check 'copy' cli arguments. - checkCopySyntax(ctx, cliCtx, encKeyDB) + checkCopySyntax(cliCtx) if cliCtx.NArg() == 2 { args := cliCtx.Args() diff --git a/cmd/typed-errors.go b/cmd/typed-errors.go index cd00ec8b10..b2c7648752 100644 --- a/cmd/typed-errors.go +++ b/cmd/typed-errors.go @@ -111,11 +111,11 @@ var errRequiresRecursive = func(URL string) *probe.Error { return probe.NewError(requiresRecuriveErr(errors.New(msg))).Untrace() } -type CopyIntoSelfErr error +type copyIntoSelfErr error var errCopyIntoSelf = func(URL string) *probe.Error { msg := "Copying or moving '" + URL + "' into itself is not allowed." - return probe.NewError(CopyIntoSelfErr(errors.New(msg))).Untrace() + return probe.NewError(copyIntoSelfErr(errors.New(msg))).Untrace() } type targetNotFoundErr error From 1c9951cd5b1300e8e8eae134c110a118b19428a9 Mon Sep 17 00:00:00 2001 From: zveinn Date: Tue, 10 Oct 2023 09:32:57 +0000 Subject: [PATCH 4/8] removing comments --- cmd/cp-url.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cmd/cp-url.go b/cmd/cp-url.go index 415f23dab0..c0d6ee99eb 100644 --- a/cmd/cp-url.go +++ b/cmd/cp-url.go @@ -224,12 +224,6 @@ func prepareCopyURLsTypeC(ctx context.Context, sourceContent, targetContent *Cli } } - // if !isRecursive { - // copyURLsCh <- makeCopyContentTypeC(sourceAlias, c.GetURL(), sourceContent, targetAlias, targetURL) - // close(copyURLsCh) - // return copyURLsCh - // } - go func(sourceClient Client, sourceURL, targetURL string, copyURLsCh chan URLs) { defer close(copyURLsCh) From 77b391713c547071836c2ec4faad485cf6cbee73 Mon Sep 17 00:00:00 2001 From: zveinn Date: Tue, 10 Oct 2023 12:07:23 +0000 Subject: [PATCH 5/8] Refactor based on original PR discussion with Klaus --- cmd/cp-url.go | 226 +++++++++++++++++++++++++------------------- cmd/od-main.go | 25 +---- functional-tests.sh | 15 ++- 3 files changed, 141 insertions(+), 125 deletions(-) diff --git a/cmd/cp-url.go b/cmd/cp-url.go index c0d6ee99eb..a4b4ea45d4 100644 --- a/cmd/cp-url.go +++ b/cmd/cp-url.go @@ -19,6 +19,8 @@ package cmd import ( "context" + "fmt" + "log" "path/filepath" "strings" "time" @@ -53,181 +55,183 @@ const ( // guessCopyURLType guesses the type of clientURL. This approach all allows prepareURL // functions to accurately report failure causes. -func guessCopyURLType(ctx context.Context, o prepareCopyURLsOpts) (copyURLsType, string, *ClientContent, *ClientContent, *probe.Error) { +func guessCopyURLType(ctx context.Context, o prepareCopyURLsOpts) (*copyURLsContent, *probe.Error) { + cc := new(copyURLsContent) + + // Extract alias before fiddling with the clientURL. + cc.sourceURL = o.sourceURLs[0] + cc.sourceAlias, _, _ = mustExpandAlias(cc.sourceURL) + // Find alias and expanded clientURL. + cc.targetAlias, cc.targetURL, _ = mustExpandAlias(o.targetURL) + if len(o.sourceURLs) == 1 { // 1 Source, 1 Target var err *probe.Error - var sourceContent *ClientContent - sourceURL := o.sourceURLs[0] if !o.isRecursive { - _, sourceContent, err = url2Stat(ctx, sourceURL, o.versionID, false, o.encKeyDB, o.timeRef, o.isZip) + _, cc.sourceContent, err = url2Stat(ctx, cc.sourceURL, o.versionID, false, o.encKeyDB, o.timeRef, o.isZip) } else { - _, sourceContent, err = firstURL2Stat(ctx, sourceURL, o.timeRef, o.isZip) + _, cc.sourceContent, err = firstURL2Stat(ctx, cc.sourceURL, o.timeRef, o.isZip) } + if err != nil { - return copyURLsTypeInvalid, "", sourceContent, nil, err + cc.copyType = copyURLsTypeInvalid + return cc, err } // If recursion is ON, it is type C. // If source is a folder, it is Type C. - if sourceContent.Type.IsDir() || o.isRecursive { - return copyURLsTypeC, "", sourceContent, nil, nil + if cc.sourceContent.Type.IsDir() || o.isRecursive { + cc.copyType = copyURLsTypeC + return cc, nil } // If target is a folder, it is Type B. - isDir, targetContent := isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) + var isDir bool + isDir, cc.targetContent = isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) if isDir { - return copyURLsTypeB, sourceContent.VersionID, sourceContent, targetContent, nil + cc.copyType = copyURLsTypeB + cc.sourceVersionID = cc.sourceContent.VersionID + return cc, nil } + // else Type A. - return copyURLsTypeA, sourceContent.VersionID, sourceContent, targetContent, nil + cc.copyType = copyURLsTypeA + cc.sourceVersionID = cc.sourceContent.VersionID + return cc, nil } + var isDir bool // Multiple source args and target is a folder. It is Type D. - isDir, targetContent := isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) + isDir, cc.targetContent = isAliasURLDir(ctx, o.targetURL, o.encKeyDB, o.timeRef) if isDir { - return copyURLsTypeD, "", nil, targetContent, nil + cc.copyType = copyURLsTypeD + return cc, nil } - return copyURLsTypeInvalid, "", nil, nil, errInvalidArgument().Trace() + cc.copyType = copyURLsTypeInvalid + return cc, errInvalidArgument().Trace() } // SINGLE SOURCE - Type A: copy(f, f) -> copy(f, f) // prepareCopyURLsTypeA - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeA(ctx context.Context, sourceContent *ClientContent, sourceURL, sourceVersion, targetURL string, encKeyDB map[string][]prefixSSEPair, isZip bool) URLs { - // Extract alias before fiddling with the clientURL. - sourceAlias, _, _ := mustExpandAlias(sourceURL) - // Find alias and expanded clientURL. - targetAlias, targetURL, _ := mustExpandAlias(targetURL) - +func prepareCopyURLsTypeA(ctx context.Context, cc copyURLsContent, o prepareCopyURLsOpts) URLs { var err *probe.Error - if sourceContent == nil { - _, sourceContent, err = url2Stat(ctx, sourceURL, sourceVersion, false, encKeyDB, time.Time{}, isZip) + if cc.sourceContent == nil { + _, cc.sourceContent, err = url2Stat(ctx, cc.sourceURL, cc.sourceVersionID, false, o.encKeyDB, time.Time{}, o.isZip) if err != nil { // Source does not exist or insufficient privileges. - return URLs{Error: err.Trace(sourceURL)} + return URLs{Error: err.Trace(cc.sourceURL)} } } - if !sourceContent.Type.IsRegular() { + if !cc.sourceContent.Type.IsRegular() { // Source is not a regular file - return URLs{Error: errInvalidSource(sourceURL).Trace(sourceURL)} + return URLs{Error: errInvalidSource(cc.sourceURL).Trace(cc.sourceURL)} } - // All OK.. We can proceed. Type A - return makeCopyContentTypeA(sourceAlias, sourceContent, targetAlias, targetURL) + return makeCopyContentTypeA(cc) } // prepareCopyContentTypeA - makes CopyURLs content for copying. -func makeCopyContentTypeA(sourceAlias string, sourceContent *ClientContent, targetAlias, targetURL string) URLs { - targetContent := ClientContent{URL: *newClientURL(targetURL)} +func makeCopyContentTypeA(cc copyURLsContent) URLs { + targetContent := ClientContent{URL: *newClientURL(cc.targetURL)} return URLs{ - SourceAlias: sourceAlias, - SourceContent: sourceContent, - TargetAlias: targetAlias, + SourceAlias: cc.sourceAlias, + SourceContent: cc.sourceContent, + TargetAlias: cc.targetAlias, TargetContent: &targetContent, } } // SINGLE SOURCE - Type B: copy(f, d) -> copy(f, d/f) -> A // prepareCopyURLsTypeB - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeB(ctx context.Context, sourceContent, targetContent *ClientContent, sourceURL, sourceVersion, targetURL string, encKeyDB map[string][]prefixSSEPair, isZip bool) URLs { - // Extract alias before fiddling with the clientURL. - sourceAlias, _, _ := mustExpandAlias(sourceURL) - // Find alias and expanded clientURL. - targetAlias, targetURL, _ := mustExpandAlias(targetURL) - +func prepareCopyURLsTypeB(ctx context.Context, cc copyURLsContent, o prepareCopyURLsOpts) URLs { var err *probe.Error - if sourceContent == nil { - _, sourceContent, err = url2Stat(ctx, sourceURL, sourceVersion, false, encKeyDB, time.Time{}, isZip) + if cc.sourceContent == nil { + _, cc.sourceContent, err = url2Stat(ctx, cc.sourceURL, cc.sourceVersionID, false, o.encKeyDB, time.Time{}, o.isZip) if err != nil { // Source does not exist or insufficient privileges. - return URLs{Error: err.Trace(sourceURL)} + return URLs{Error: err.Trace(cc.sourceURL)} } } - if !sourceContent.Type.IsRegular() { - if sourceContent.Type.IsDir() { - return URLs{Error: errSourceIsDir(sourceURL).Trace(sourceURL)} + if !cc.sourceContent.Type.IsRegular() { + if cc.sourceContent.Type.IsDir() { + return URLs{Error: errSourceIsDir(cc.sourceURL).Trace(cc.sourceURL)} } // Source is not a regular file. - return URLs{Error: errInvalidSource(sourceURL).Trace(sourceURL)} + return URLs{Error: errInvalidSource(cc.sourceURL).Trace(cc.sourceURL)} } - if targetContent == nil { - _, targetContent, err = url2Stat(ctx, targetURL, "", false, encKeyDB, time.Time{}, false) + if cc.targetContent == nil { + _, cc.targetContent, err = url2Stat(ctx, cc.targetURL, "", false, o.encKeyDB, time.Time{}, false) if err == nil { - if !targetContent.Type.IsDir() { - return URLs{Error: errInvalidTarget(targetURL).Trace(targetURL)} + if !cc.targetContent.Type.IsDir() { + return URLs{Error: errInvalidTarget(cc.targetURL).Trace(cc.targetURL)} } } } - // All OK.. We can proceed. Type B: source is a file, target is a folder and exists. - return makeCopyContentTypeB(sourceAlias, sourceContent, targetAlias, targetURL) + return makeCopyContentTypeB(cc) } // makeCopyContentTypeB - CopyURLs content for copying. -func makeCopyContentTypeB(sourceAlias string, sourceContent *ClientContent, targetAlias, targetURL string) URLs { +func makeCopyContentTypeB(cc copyURLsContent) URLs { // All OK.. We can proceed. Type B: source is a file, target is a folder and exists. - targetURLParse := newClientURL(targetURL) - targetURLParse.Path = filepath.ToSlash(filepath.Join(targetURLParse.Path, filepath.Base(sourceContent.URL.Path))) - return makeCopyContentTypeA(sourceAlias, sourceContent, targetAlias, targetURLParse.String()) + targetURLParse := newClientURL(cc.targetURL) + targetURLParse.Path = filepath.ToSlash(filepath.Join(targetURLParse.Path, filepath.Base(cc.sourceContent.URL.Path))) + cc.targetURL = targetURLParse.String() + return makeCopyContentTypeA(cc) } // SINGLE SOURCE - Type C: copy(d1..., d2) -> []copy(d1/f, d1/d2/f) -> []A // prepareCopyRecursiveURLTypeC - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeC(ctx context.Context, sourceContent, targetContent *ClientContent, sourceURL, targetURL string, encKeyDB map[string][]prefixSSEPair, isRecursive, isZip bool, timeRef time.Time) <-chan URLs { - // Extract alias before fiddling with the clientURL. - sourceAlias, _, _ := mustExpandAlias(sourceURL) - // Find alias and expanded clientURL. - targetAlias, targetURL, _ := mustExpandAlias(targetURL) +func prepareCopyURLsTypeC(ctx context.Context, cc copyURLsContent, o prepareCopyURLsOpts) <-chan URLs { copyURLsCh := make(chan URLs, 1) + fmt.Printf("POINTER TO CC: %p >> URL: %s\n", &cc, cc.sourceURL) returnErrorAndCloseChannel := func(err *probe.Error) chan URLs { copyURLsCh <- URLs{Error: err} close(copyURLsCh) return copyURLsCh } - c, err := newClient(sourceURL) + c, err := newClient(cc.sourceURL) if err != nil { - return returnErrorAndCloseChannel(err.Trace(sourceURL)) + return returnErrorAndCloseChannel(err.Trace(cc.sourceURL)) } - if targetContent == nil { - _, targetContent, err = url2Stat(ctx, targetURL, "", false, encKeyDB, time.Time{}, isZip) - if err != nil { - return returnErrorAndCloseChannel(err.Trace(targetURL)) + if cc.targetContent == nil { + _, cc.targetContent, err = url2Stat(ctx, cc.targetURL, "", false, o.encKeyDB, time.Time{}, o.isZip) + if err == nil { + if !cc.targetContent.Type.IsDir() { + return returnErrorAndCloseChannel(errTargetIsNotDir(cc.targetURL).Trace(cc.targetURL)) + } } } - if !targetContent.Type.IsDir() { - return returnErrorAndCloseChannel(errTargetIsNotDir(targetURL).Trace(targetURL)) - } - - if sourceContent == nil { - _, sourceContent, err = url2Stat(ctx, sourceURL, "", false, encKeyDB, time.Time{}, isZip) + if cc.sourceContent == nil { + _, cc.sourceContent, err = url2Stat(ctx, cc.sourceURL, "", false, o.encKeyDB, time.Time{}, o.isZip) if err != nil { - return returnErrorAndCloseChannel(err.Trace(sourceURL)) + return returnErrorAndCloseChannel(err.Trace(cc.sourceURL)) } } - if sourceContent.Type.IsDir() { + if cc.sourceContent.Type.IsDir() { // Require --recursive flag if we are copying a directory - if !isRecursive { - return returnErrorAndCloseChannel(errRequiresRecursive(sourceURL).Trace(sourceURL)) + if !o.isRecursive { + return returnErrorAndCloseChannel(errRequiresRecursive(cc.sourceURL).Trace(cc.sourceURL)) } // Check if we are going to copy a directory into itself - if isURLContains(sourceURL, targetURL, string(c.GetURL().Separator)) { - return returnErrorAndCloseChannel(errCopyIntoSelf(sourceURL).Trace(targetURL)) + if isURLContains(cc.sourceURL, cc.targetURL, string(c.GetURL().Separator)) { + return returnErrorAndCloseChannel(errCopyIntoSelf(cc.sourceURL).Trace(cc.targetURL)) } } - go func(sourceClient Client, sourceURL, targetURL string, copyURLsCh chan URLs) { + go func(sourceClient Client, cc copyURLsContent, o prepareCopyURLsOpts, copyURLsCh chan URLs) { defer close(copyURLsCh) - for sourceContent := range sourceClient.List(ctx, ListOptions{Recursive: isRecursive, TimeRef: timeRef, ShowDir: DirNone, ListZip: isZip}) { + for sourceContent := range sourceClient.List(ctx, ListOptions{Recursive: o.isRecursive, TimeRef: o.timeRef, ShowDir: DirNone, ListZip: o.isZip}) { if sourceContent.Err != nil { // Listing failed. copyURLsCh <- URLs{Error: sourceContent.Err.Trace(sourceClient.GetURL().String())} @@ -239,40 +243,49 @@ func prepareCopyURLsTypeC(ctx context.Context, sourceContent, targetContent *Cli continue } + // Clone cc + newCC := cc + newCC.sourceContent = sourceContent // All OK.. We can proceed. Type B: source is a file, target is a folder and exists. - copyURLsCh <- makeCopyContentTypeC(sourceAlias, sourceClient.GetURL(), sourceContent, targetAlias, targetURL) + copyURLsCh <- makeCopyContentTypeC(newCC, sourceClient.GetURL()) } - }(c, sourceURL, targetURL, copyURLsCh) + }(c, cc, o, copyURLsCh) return copyURLsCh } // makeCopyContentTypeC - CopyURLs content for copying. -func makeCopyContentTypeC(sourceAlias string, sourceURL ClientURL, sourceContent *ClientContent, targetAlias, targetURL string) URLs { - newSourceURL := sourceContent.URL - pathSeparatorIndex := strings.LastIndex(sourceURL.Path, string(sourceURL.Separator)) +func makeCopyContentTypeC(cc copyURLsContent, sourceClientURL ClientURL) URLs { + newSourceURL := cc.sourceContent.URL + pathSeparatorIndex := strings.LastIndex(sourceClientURL.Path, string(sourceClientURL.Separator)) newSourceSuffix := filepath.ToSlash(newSourceURL.Path) if pathSeparatorIndex > 1 { - sourcePrefix := filepath.ToSlash(sourceURL.Path[:pathSeparatorIndex]) + sourcePrefix := filepath.ToSlash(sourceClientURL.Path[:pathSeparatorIndex]) newSourceSuffix = strings.TrimPrefix(newSourceSuffix, sourcePrefix) } - newTargetURL := urlJoinPath(targetURL, newSourceSuffix) - return makeCopyContentTypeA(sourceAlias, sourceContent, targetAlias, newTargetURL) + newTargetURL := urlJoinPath(cc.targetURL, newSourceSuffix) + cc.targetURL = newTargetURL + return makeCopyContentTypeA(cc) } // MULTI-SOURCE - Type D: copy([](f|d...), d) -> []B // prepareCopyURLsTypeE - prepares target and source clientURLs for copying. -func prepareCopyURLsTypeD(ctx context.Context, sourceContent *ClientContent, targetContent *ClientContent, sourceURLs []string, targetURL string, encKeyDB map[string][]prefixSSEPair, isRecursive bool, timeRef time.Time, isZip bool) <-chan URLs { +func prepareCopyURLsTypeD(ctx context.Context, cc copyURLsContent, o prepareCopyURLsOpts) <-chan URLs { copyURLsCh := make(chan URLs, 1) - go func(sourceURLs []string, sourceContent, targetContent *ClientContent, targetURL string, copyURLsCh chan URLs) { + go func(ctx context.Context, cc copyURLsContent, o prepareCopyURLsOpts) { defer close(copyURLsCh) - for _, sourceURL := range sourceURLs { - for cpURLs := range prepareCopyURLsTypeC(ctx, nil, targetContent, sourceURL, targetURL, encKeyDB, isRecursive, isZip, timeRef) { + + for _, sourceURL := range o.sourceURLs { + // Clone CC + newCC := cc + newCC.sourceURL = sourceURL + + for cpURLs := range prepareCopyURLsTypeC(ctx, newCC, o) { copyURLsCh <- cpURLs } } - }(sourceURLs, sourceContent, targetContent, targetURL, copyURLsCh) + }(ctx, cc, o) return copyURLsCh } @@ -288,28 +301,43 @@ type prepareCopyURLsOpts struct { isZip bool } +type copyURLsContent struct { + targetContent *ClientContent + targetAlias string + targetURL string + sourceContent *ClientContent + sourceAlias string + sourceURL string + copyType copyURLsType + sourceVersionID string +} + // prepareCopyURLs - prepares target and source clientURLs for copying. func prepareCopyURLs(ctx context.Context, o prepareCopyURLsOpts) chan URLs { copyURLsCh := make(chan URLs) go func(o prepareCopyURLsOpts) { defer close(copyURLsCh) - cpType, cpVersion, sourceContent, targetContent, err := guessCopyURLType(ctx, o) + copyURLsContent, err := guessCopyURLType(ctx, o) if err != nil { copyURLsCh <- URLs{Error: errUnableToGuess().Trace(o.sourceURLs...)} return } - switch cpType { + switch copyURLsContent.copyType { case copyURLsTypeA: - copyURLsCh <- prepareCopyURLsTypeA(ctx, sourceContent, o.sourceURLs[0], cpVersion, o.targetURL, o.encKeyDB, o.isZip) + log.Println("TYPE A") + copyURLsCh <- prepareCopyURLsTypeA(ctx, *copyURLsContent, o) case copyURLsTypeB: - copyURLsCh <- prepareCopyURLsTypeB(ctx, sourceContent, targetContent, o.sourceURLs[0], cpVersion, o.targetURL, o.encKeyDB, o.isZip) + log.Println("TYPE B") + copyURLsCh <- prepareCopyURLsTypeB(ctx, *copyURLsContent, o) case copyURLsTypeC: - for cURLs := range prepareCopyURLsTypeC(ctx, sourceContent, targetContent, o.sourceURLs[0], o.targetURL, o.encKeyDB, o.isRecursive, o.isZip, o.timeRef) { + log.Println("TYPE C") + for cURLs := range prepareCopyURLsTypeC(ctx, *copyURLsContent, o) { copyURLsCh <- cURLs } case copyURLsTypeD: - for cURLs := range prepareCopyURLsTypeD(ctx, sourceContent, targetContent, o.sourceURLs, o.targetURL, o.encKeyDB, o.isRecursive, o.timeRef, o.isZip) { + log.Println("TYPE D") + for cURLs := range prepareCopyURLsTypeD(ctx, *copyURLsContent, o) { copyURLsCh <- cURLs } default: diff --git a/cmd/od-main.go b/cmd/od-main.go index fd012cc38f..36a9f2e8fb 100644 --- a/cmd/od-main.go +++ b/cmd/od-main.go @@ -105,13 +105,13 @@ func getOdUrls(ctx context.Context, args argKVS) (odURLs URLs, e error) { sourceURLs: []string{inFile}, targetURL: outFile, } - odType, _, _, _, err := guessCopyURLType(ctx, opts) + copyURLsContent, err := guessCopyURLType(ctx, opts) fatalIf(err, "Unable to guess copy URL type") // Get content of inFile, set up URLs. - switch odType { + switch copyURLsContent.copyType { case copyURLsTypeA: - odURLs = prepareOdUrls(ctx, inFile, "", outFile) + odURLs = makeCopyContentTypeA(*copyURLsContent) case copyURLsTypeB: return URLs{}, fmt.Errorf("invalid source path %s, destination cannot be a directory", outFile) default: @@ -121,25 +121,6 @@ func getOdUrls(ctx context.Context, args argKVS) (odURLs URLs, e error) { return odURLs, nil } -func prepareOdUrls(ctx context.Context, sourceURL, sourceVersion, targetURL string) URLs { - // Extract alias before fiddling with the clientURL. - sourceAlias, _, _ := mustExpandAlias(sourceURL) - // Find alias and expanded clientURL. - targetAlias, targetURL, _ := mustExpandAlias(targetURL) - - // Placeholder encryption key database - var encKeyDB map[string][]prefixSSEPair - - _, sourceContent, err := url2Stat(ctx, sourceURL, sourceVersion, false, encKeyDB, time.Time{}, false) - if err != nil { - // Source does not exist or insufficient privileges. - return URLs{Error: err} - } - - // All OK.. We can proceed. Type A - return makeCopyContentTypeA(sourceAlias, sourceContent, targetAlias, targetURL) -} - // odCheckType checks if request is a download or upload and calls the appropriate function func odCheckType(ctx context.Context, odURLs URLs, args argKVS) (message, error) { if odURLs.SourceAlias != "" && odURLs.TargetAlias == "" { diff --git a/functional-tests.sh b/functional-tests.sh index b7d19893fe..5f9e6029e1 100755 --- a/functional-tests.sh +++ b/functional-tests.sh @@ -315,6 +315,16 @@ function test_list_dir() log_success "$start_time" "${FUNCNAME[0]}" } +function test_od_object() { + show "${FUNCNAME[0]}" + + start_time=$(get_time) + object_name="mc-test-object-$RANDOM" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd od if="${FILE_1_MB}" of="${SERVER_ALIAS}/${BUCKET_NAME}/${object_name}" + assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd od of="${FILE_1_MB}" if="${SERVER_ALIAS}/${BUCKET_NAME}/${object_name}" + + log_success "$start_time" "${FUNCNAME[0]}" +} function test_put_object() { @@ -984,9 +994,6 @@ function test_admin_users() username=foo password=foobar12345 test_alias="aliasx" - - # Remove user incase the user was created and not removed due to a failed test - mc_cmd admin user remove "$SERVER_ALIAS" "$username" assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd admin user add "$SERVER_ALIAS" "$username" "$password" @@ -1019,7 +1026,6 @@ function test_admin_users() rv=$? assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure ${rv} "user ${username} did NOT have the readwrite policy attached" - assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd mb "${test_alias}/${BUCKET_NAME}" assert_success "$start_time" "${FUNCNAME[0]}" mc_cmd cp "$FILE_1_MB" "${test_alias}/${BUCKET_NAME}/${object1_name}" # check that the user cannot write objects with readonly policy @@ -1074,6 +1080,7 @@ function run_test() test_put_object_multipart test_get_object test_get_object_multipart + test_od_object test_mv_object test_presigned_post_policy_error test_presigned_put_object From 3662c552346e415dc99843c1e72ec9086df01456 Mon Sep 17 00:00:00 2001 From: zveinn Date: Tue, 10 Oct 2023 12:26:49 +0000 Subject: [PATCH 6/8] Removing printlns --- cmd/cp-url.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cmd/cp-url.go b/cmd/cp-url.go index a4b4ea45d4..0c5450deda 100644 --- a/cmd/cp-url.go +++ b/cmd/cp-url.go @@ -20,7 +20,6 @@ package cmd import ( "context" "fmt" - "log" "path/filepath" "strings" "time" @@ -325,18 +324,14 @@ func prepareCopyURLs(ctx context.Context, o prepareCopyURLsOpts) chan URLs { switch copyURLsContent.copyType { case copyURLsTypeA: - log.Println("TYPE A") copyURLsCh <- prepareCopyURLsTypeA(ctx, *copyURLsContent, o) case copyURLsTypeB: - log.Println("TYPE B") copyURLsCh <- prepareCopyURLsTypeB(ctx, *copyURLsContent, o) case copyURLsTypeC: - log.Println("TYPE C") for cURLs := range prepareCopyURLsTypeC(ctx, *copyURLsContent, o) { copyURLsCh <- cURLs } case copyURLsTypeD: - log.Println("TYPE D") for cURLs := range prepareCopyURLsTypeD(ctx, *copyURLsContent, o) { copyURLsCh <- cURLs } From d487b7e55302abec3d5a207707dcf20103151391 Mon Sep 17 00:00:00 2001 From: zveinn Date: Tue, 10 Oct 2023 12:28:17 +0000 Subject: [PATCH 7/8] Removing the last print, yikes --- cmd/cp-url.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/cp-url.go b/cmd/cp-url.go index 0c5450deda..4bff8ba1f2 100644 --- a/cmd/cp-url.go +++ b/cmd/cp-url.go @@ -19,7 +19,6 @@ package cmd import ( "context" - "fmt" "path/filepath" "strings" "time" @@ -187,7 +186,6 @@ func makeCopyContentTypeB(cc copyURLsContent) URLs { func prepareCopyURLsTypeC(ctx context.Context, cc copyURLsContent, o prepareCopyURLsOpts) <-chan URLs { copyURLsCh := make(chan URLs, 1) - fmt.Printf("POINTER TO CC: %p >> URL: %s\n", &cc, cc.sourceURL) returnErrorAndCloseChannel := func(err *probe.Error) chan URLs { copyURLsCh <- URLs{Error: err} close(copyURLsCh) From cb7a9fe62058dfda5b24f881bac4e5bf5c7f3f7e Mon Sep 17 00:00:00 2001 From: zveinn Date: Tue, 10 Oct 2023 17:33:42 +0000 Subject: [PATCH 8/8] fixing tab indentation --- functional-tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/functional-tests.sh b/functional-tests.sh index 5f9e6029e1..b6961f6e41 100755 --- a/functional-tests.sh +++ b/functional-tests.sh @@ -1198,7 +1198,7 @@ function validate_dependencies() { function main() { - validate_dependencies + validate_dependencies ( run_test ) rv=$?