From 27831c7e461463812ccf78faae97225f4560beee Mon Sep 17 00:00:00 2001 From: yahavi Date: Thu, 23 Nov 2023 15:40:28 +0200 Subject: [PATCH] Transfer - Fix and improve long props check --- .../transferfiles/longpropertycheck.go | 41 +++++++++------- .../transferfiles/longpropertycheck_test.go | 48 +++++++++++-------- .../commands/transferfiles/transfer.go | 10 ++-- .../utils/precheckrunner/remoteurlchecker.go | 4 +- 4 files changed, 60 insertions(+), 43 deletions(-) diff --git a/artifactory/commands/transferfiles/longpropertycheck.go b/artifactory/commands/transferfiles/longpropertycheck.go index d3135b08c..f22da8e93 100644 --- a/artifactory/commands/transferfiles/longpropertycheck.go +++ b/artifactory/commands/transferfiles/longpropertycheck.go @@ -10,6 +10,7 @@ import ( "github.com/jfrog/jfrog-cli-core/v2/artifactory/commands/transferfiles/api" cmdutils "github.com/jfrog/jfrog-cli-core/v2/artifactory/commands/utils" "github.com/jfrog/jfrog-cli-core/v2/artifactory/commands/utils/precheckrunner" + "github.com/jfrog/jfrog-cli-core/v2/artifactory/utils" "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" "github.com/jfrog/jfrog-cli-core/v2/utils/progressbar" "github.com/jfrog/jfrog-client-go/artifactory" @@ -30,6 +31,7 @@ const ( threadCount = 10 maxAllowedValLength = 2400 longPropertyCheckName = "Properties with value longer than 2.4K characters" + propertiesRequestTimeout = time.Minute * 30 ) // Property - Represents a property of an item @@ -54,14 +56,15 @@ type FileWithLongProperty struct { } type LongPropertyCheck struct { - producerConsumer parallel.Runner - filesChan chan FileWithLongProperty - errorsQueue *clientutils.ErrorsQueue - repos []string + producerConsumer parallel.Runner + filesChan chan FileWithLongProperty + errorsQueue *clientutils.ErrorsQueue + repos []string + disabledDistinctiveAql bool } -func NewLongPropertyCheck(repos []string) *LongPropertyCheck { - return &LongPropertyCheck{repos: repos} +func NewLongPropertyCheck(repos []string, disabledDistinctiveAql bool) *LongPropertyCheck { + return &LongPropertyCheck{repos: repos, disabledDistinctiveAql: disabledDistinctiveAql} } func (lpc *LongPropertyCheck) Name() string { @@ -114,7 +117,7 @@ func (lpc *LongPropertyCheck) ExecuteCheck(args precheckrunner.RunArguments) (pa // Returns the number of long properties found func (lpc *LongPropertyCheck) longPropertiesTaskProducer(progress *progressbar.TasksProgressBar, args precheckrunner.RunArguments) int { // Init - serviceManager, err := createTransferServiceManager(args.Context, args.ServerDetails) + serviceManager, err := utils.CreateServiceManagerWithContext(args.Context, args.ServerDetails, false, 0, retries, retriesWaitMilliSecs, propertiesRequestTimeout) if err != nil { return 0 } @@ -131,7 +134,7 @@ func (lpc *LongPropertyCheck) longPropertiesTaskProducer(progress *progressbar.T if long := isLongProperty(property); long { log.Debug(fmt.Sprintf(`Found long property ('@%s':'%s')`, property.Key, property.Value)) if lpc.producerConsumer != nil { - _, _ = lpc.producerConsumer.AddTaskWithError(createSearchPropertyTask(property, lpc.repos, args, lpc.filesChan, progress), lpc.errorsQueue.AddError) + _, _ = lpc.producerConsumer.AddTaskWithError(lpc.createSearchPropertyTask(property, args, progress), lpc.errorsQueue.AddError) } if progress != nil { progress.IncGeneralProgressTotalBy(1) @@ -174,25 +177,25 @@ func getSearchAllPropertiesQuery(pageNumber int) string { // Create a task that fetch from the server the files with the given property. // We keep only the files that are at the requested repos and pass them at the files channel -func createSearchPropertyTask(property Property, repos []string, args precheckrunner.RunArguments, filesChan chan FileWithLongProperty, progress *progressbar.TasksProgressBar) parallel.TaskFunc { +func (lpc *LongPropertyCheck) createSearchPropertyTask(property Property, args precheckrunner.RunArguments, progress *progressbar.TasksProgressBar) parallel.TaskFunc { return func(threadId int) (err error) { - serviceManager, err := createTransferServiceManager(args.Context, args.ServerDetails) + serviceManager, err := utils.CreateServiceManagerWithContext(args.Context, args.ServerDetails, false, 0, retries, retriesWaitMilliSecs, propertiesRequestTimeout) if err != nil { return } // Search var query *servicesUtils.AqlSearchResult - if query, err = runSearchPropertyInFilesAql(serviceManager, property); err != nil { + if query, err = lpc.runSearchPropertyInFilesAql(serviceManager, property); err != nil { return } log.Debug(fmt.Sprintf("[Thread=%d] Got %d files from the query", threadId, len(query.Results))) for _, item := range query.Results { file := api.FileRepresentation{Repo: item.Repo, Path: item.Path, Name: item.Name} // Keep only if in the requested repos - if slices.Contains(repos, file.Repo) { + if slices.Contains(lpc.repos, file.Repo) { fileWithLongProperty := FileWithLongProperty{file, property.valueLength(), property} log.Debug(fmt.Sprintf("[Thread=%d] Found File{Repo=%s, Path=%s, Name=%s} with matching entry of long property.", threadId, file.Repo, file.Path, file.Name)) - filesChan <- fileWithLongProperty + lpc.filesChan <- fileWithLongProperty } } // Notify end of search for the current property @@ -204,15 +207,19 @@ func createSearchPropertyTask(property Property, repos []string, args precheckru } // Get all the files that contains the given property using AQL -func runSearchPropertyInFilesAql(serviceManager artifactory.ArtifactoryServicesManager, property Property) (result *servicesUtils.AqlSearchResult, err error) { +func (lpc *LongPropertyCheck) runSearchPropertyInFilesAql(serviceManager artifactory.ArtifactoryServicesManager, property Property) (result *servicesUtils.AqlSearchResult, err error) { result = &servicesUtils.AqlSearchResult{} - err = runAqlService(serviceManager, getSearchPropertyInFilesQuery(property), result) + err = runAqlService(serviceManager, lpc.getSearchPropertyInFilesQuery(property), result) return } // Get the query that search files with specific property -func getSearchPropertyInFilesQuery(property Property) string { - return fmt.Sprintf(`items.find({"type": {"$eq":"any"},"@%s":"%s"}).include("repo","path","name")`, property.Key, property.Value) +func (lpc *LongPropertyCheck) getSearchPropertyInFilesQuery(property Property) string { + query := fmt.Sprintf(`items.find({"type": {"$eq":"any"},"@%s":"%s"}).include("repo","path","name")`, property.Key, property.Value) + if lpc.disabledDistinctiveAql { + query += `.distinct(false)` + } + return query } // Run AQL service that return a result in the given format structure 'v' diff --git a/artifactory/commands/transferfiles/longpropertycheck_test.go b/artifactory/commands/transferfiles/longpropertycheck_test.go index 0519dbbca..6b1ac938f 100644 --- a/artifactory/commands/transferfiles/longpropertycheck_test.go +++ b/artifactory/commands/transferfiles/longpropertycheck_test.go @@ -77,10 +77,10 @@ func TestGetLongProperties(t *testing.T) { } func testGetLongProperties(t *testing.T, serverProperties, expectedLongProperties []Property) { - testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, serverProperties, propertyToFiles) + testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, serverProperties, propertyToFiles, false) defer testServer.Close() - longPropertyCheck := NewLongPropertyCheck([]string{}) + longPropertyCheck := NewLongPropertyCheck([]string{}, true) longPropertyCheck.filesChan = make(chan FileWithLongProperty, threadCount) count := longPropertyCheck.longPropertiesTaskProducer(nil, precheckrunner.RunArguments{Context: nil, ServerDetails: serverDetails}) @@ -105,7 +105,7 @@ func TestSearchPropertyInFilesTask(t *testing.T) { } func testSearchPropertyInFilesTask(t *testing.T, prop Property, specificRepos []string, propertiesFiles map[Property][]api.FileRepresentation, expected []FileWithLongProperty) { - testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, nil, propertiesFiles) + testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, nil, propertiesFiles, false) defer testServer.Close() var result []FileWithLongProperty @@ -120,7 +120,11 @@ func testSearchPropertyInFilesTask(t *testing.T, prop Property, specificRepos [] wait.Done() }() - task := createSearchPropertyTask(prop, specificRepos, precheckrunner.RunArguments{Context: nil, ServerDetails: serverDetails}, filesChan, nil) + longPropertyCheck := LongPropertyCheck{ + filesChan: filesChan, + repos: specificRepos, + } + task := longPropertyCheck.createSearchPropertyTask(prop, precheckrunner.RunArguments{Context: nil, ServerDetails: serverDetails}, nil) assert.NoError(t, task(0)) close(filesChan) wait.Wait() @@ -152,10 +156,10 @@ func TestSearchLongPropertiesInFiles(t *testing.T) { } func testSearchPropertiesInFiles(t *testing.T, properties []Property, specificRepos []string, propertiesFiles map[Property][]api.FileRepresentation, expected []FileWithLongProperty) { - testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, properties, propertiesFiles) + testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, properties, propertiesFiles, false) defer testServer.Close() - longPropertyCheck := NewLongPropertyCheck(specificRepos) + longPropertyCheck := NewLongPropertyCheck(specificRepos, true) longPropertyCheck.producerConsumer = parallel.NewRunner(threadCount, maxThreadCapacity, false) longPropertyCheck.filesChan = make(chan FileWithLongProperty, threadCount) longPropertyCheck.errorsQueue = clientutils.NewErrorsQueue(1) @@ -181,33 +185,34 @@ func testSearchPropertiesInFiles(t *testing.T, properties []Property, specificRe func TestLongPropertyExecuteCheck(t *testing.T) { cases := []struct { - serverProperties []Property - specificRepos []string - shouldPass bool + serverProperties []Property + specificRepos []string + disabledDistinctiveAql bool + shouldPass bool }{ - {[]Property{}, []string{"Repo", "OtherRepo"}, true}, - {[]Property{property, shorterProperty, borderProperty}, []string{"Repo", "OtherRepo"}, true}, - {[]Property{property, shorterProperty, borderProperty, longProperty, longProperty2}, []string{"Repo", "OtherRepo"}, false}, - {[]Property{property, shorterProperty, borderProperty, longProperty2}, []string{"Repo", "OtherRepo"}, false}, - {[]Property{property, shorterProperty, borderProperty, longProperty2}, []string{"OtherRepo"}, true}, + {[]Property{}, []string{"Repo", "OtherRepo"}, true, true}, + {[]Property{property, shorterProperty, borderProperty}, []string{"Repo", "OtherRepo"}, false, true}, + {[]Property{property, shorterProperty, borderProperty, longProperty, longProperty2}, []string{"Repo", "OtherRepo"}, true, false}, + {[]Property{property, shorterProperty, borderProperty, longProperty2}, []string{"Repo", "OtherRepo"}, false, false}, + {[]Property{property, shorterProperty, borderProperty, longProperty2}, []string{"OtherRepo"}, true, true}, } for _, testCase := range cases { - testLongPropertyCheckWithStubServer(t, testCase.serverProperties, testCase.specificRepos, propertyToFiles, testCase.shouldPass) + testLongPropertyCheckWithStubServer(t, testCase.serverProperties, testCase.specificRepos, propertyToFiles, testCase.disabledDistinctiveAql, testCase.shouldPass) } } -func testLongPropertyCheckWithStubServer(t *testing.T, serverProperties []Property, specificRepos []string, propertiesFiles map[Property][]api.FileRepresentation, shouldPass bool) { - testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, serverProperties, propertiesFiles) +func testLongPropertyCheckWithStubServer(t *testing.T, serverProperties []Property, specificRepos []string, propertiesFiles map[Property][]api.FileRepresentation, disabledDistinctiveAql, shouldPass bool) { + testServer, serverDetails, _ := getLongPropertyCheckStubServer(t, serverProperties, propertiesFiles, disabledDistinctiveAql) defer testServer.Close() - longPropertyCheck := NewLongPropertyCheck(specificRepos) + longPropertyCheck := NewLongPropertyCheck(specificRepos, disabledDistinctiveAql) passed, err := longPropertyCheck.ExecuteCheck(precheckrunner.RunArguments{Context: nil, ServerDetails: serverDetails}) assert.NoError(t, err) assert.Equal(t, shouldPass, passed) } -func getLongPropertyCheckStubServer(t *testing.T, serverProperties []Property, propertiesFiles map[Property][]api.FileRepresentation) (*httptest.Server, *config.ServerDetails, artifactory.ArtifactoryServicesManager) { +func getLongPropertyCheckStubServer(t *testing.T, serverProperties []Property, propertiesFiles map[Property][]api.FileRepresentation, disabledDistinctiveAql bool) (*httptest.Server, *config.ServerDetails, artifactory.ArtifactoryServicesManager) { return commonTests.CreateRtRestsMockServer(t, func(w http.ResponseWriter, r *http.Request) { if r.RequestURI == "/"+"api/search/aql" { content, err := io.ReadAll(r.Body) @@ -238,6 +243,11 @@ func getLongPropertyCheckStubServer(t *testing.T, serverProperties []Property, p assert.NoError(t, err) _, err = w.Write(content) assert.NoError(t, err) + if disabledDistinctiveAql { + assert.Contains(t, strContent, ".distinct(false)") + } else { + assert.NotContains(t, strContent, ".distinct(false)") + } } } }) diff --git a/artifactory/commands/transferfiles/transfer.go b/artifactory/commands/transferfiles/transfer.go index d2ab16c6a..a3f168a42 100644 --- a/artifactory/commands/transferfiles/transfer.go +++ b/artifactory/commands/transferfiles/transfer.go @@ -144,6 +144,10 @@ func (tdc *TransferFilesCommand) Run() (err error) { return err } + if err = tdc.initDistinctAql(); err != nil { + return err + } + if tdc.preChecks { if runner, err := tdc.NewTransferDataPreChecksRunner(); err != nil { return err @@ -186,10 +190,6 @@ func (tdc *TransferFilesCommand) Run() (err error) { return err } - if err = tdc.initDistinctAql(); err != nil { - return err - } - if err = tdc.initStateManager(allSourceLocalRepos, sourceBuildInfoRepos); err != nil { return err } @@ -340,7 +340,7 @@ func (tdc *TransferFilesCommand) NewTransferDataPreChecksRunner() (runner *prech runner = precheckrunner.NewPreChecksRunner() // Add pre checks here - runner.AddCheck(NewLongPropertyCheck(append(localRepos, federatedRepos...))) + runner.AddCheck(NewLongPropertyCheck(append(localRepos, federatedRepos...), tdc.disabledDistinctiveAql)) return } diff --git a/artifactory/commands/utils/precheckrunner/remoteurlchecker.go b/artifactory/commands/utils/precheckrunner/remoteurlchecker.go index 173ad52f5..d06c19b99 100644 --- a/artifactory/commands/utils/precheckrunner/remoteurlchecker.go +++ b/artifactory/commands/utils/precheckrunner/remoteurlchecker.go @@ -20,7 +20,7 @@ import ( type RemoteUrlCheckStatus string const ( - longPropertyCheckName = "Remote repositories URL connectivity" + remoteUrlCheckName = "Remote repositories URL connectivity" remoteUrlCheckPollingTimeout = 30 * time.Minute remoteUrlCheckPollingInterval = 5 * time.Second remoteUrlCheckRetries = 3 @@ -61,7 +61,7 @@ func NewRemoteRepositoryCheck(targetServicesManager *artifactory.ArtifactoryServ } func (rrc *RemoteRepositoryCheck) Name() string { - return longPropertyCheckName + return remoteUrlCheckName } func (rrc *RemoteRepositoryCheck) ExecuteCheck(args RunArguments) (passed bool, err error) {