Skip to content

Commit

Permalink
Transfer - Fix and improve long props check
Browse files Browse the repository at this point in the history
  • Loading branch information
@yahavi
yahavi committed Nov 23, 2023
1 parent 6325ab6 commit 27831c7
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 43 deletions.
41 changes: 24 additions & 17 deletions artifactory/commands/transferfiles/longpropertycheck.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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'
Expand Down
48 changes: 29 additions & 19 deletions artifactory/commands/transferfiles/longpropertycheck_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand All @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)")
}
}
}
})
Expand Down
10 changes: 5 additions & 5 deletions artifactory/commands/transferfiles/transfer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions artifactory/commands/utils/precheckrunner/remoteurlchecker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 27831c7

Please sign in to comment.