From 89afa6080d59a79cd04017f83fc701d033f7909e Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Fri, 9 Jan 2026 13:39:18 +0530 Subject: [PATCH 1/9] Added e2e for DeleteCommand returns 0 for SuccessCount and FailCount when errors occur --- artifactory_test.go | 98 +++++++++++++++++++++++++++++++++++++++++++++ go.mod | 3 +- go.sum | 4 +- 3 files changed, 102 insertions(+), 3 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index 5cb271e04..6de9f54e1 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2801,6 +2801,104 @@ func TestArtifactoryDeleteByProps(t *testing.T) { cleanArtifactoryTest() } +// TestArtifactoryDeleteCountsWithPartial404 tests that delete returns accurate +// success/fail counts when 404 errors occur due to files being deleted between +// GetPathsToDelete and DeleteFiles (race condition scenario). +func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { + initArtifactoryTest(t, "") + + // Step 1: Upload more than 5 test files (using testdata/a/ which has 9 .in files) + runRt(t, "upload", "testdata/a/*.in", tests.RtRepo1+"/delete-404-test/", "--flat=true") + runRt(t, "upload", "testdata/a/b/*.in", tests.RtRepo1+"/delete-404-test/", "--flat=true") + runRt(t, "upload", "testdata/a/b/c/*.in", tests.RtRepo1+"/delete-404-test/", "--flat=true") + + // Verify we have more than 5 files uploaded + searchSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-404-test/*.in"). + BuildSpec() + searchCmd := generic.NewSearchCommand() + searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) + reader, err := searchCmd.Search() + assert.NoError(t, err) + totalFiles, err := reader.Length() + assert.NoError(t, err) + readerCloseAndAssert(t, reader) + assert.Greater(t, totalFiles, 5, "Should have uploaded more than 5 files for this test") + t.Logf("Uploaded %d files for delete-404 test", totalFiles) + + // Step 2: Create delete command and get paths to delete + deleteSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-404-test/*.in"). + BuildSpec() + + deleteCommand := generic.NewDeleteCommand() + deleteCommand.SetThreads(1). + SetSpec(deleteSpec). + SetServerDetails(serverDetails). + SetDryRun(false). + SetQuiet(true) + + // Get paths to delete (this queries Artifactory for matching files) + pathsReader, err := deleteCommand.GetPathsToDelete() + assert.NoError(t, err) + + // Verify reader has the expected number of items + readerLength, err := pathsReader.Length() + assert.NoError(t, err) + assert.Equal(t, totalFiles, readerLength, "Reader should contain all uploaded files") + + // Step 3: Simulate race condition - delete some files directly BEFORE calling DeleteFiles + // This will cause 404 errors when DeleteFiles tries to delete them + filesToPreDelete := 3 + preDeleteSpec1 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a1.in").BuildSpec() + _, _, err = tests.DeleteFiles(preDeleteSpec1, serverDetails) + assert.NoError(t, err) + + preDeleteSpec2 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a2.in").BuildSpec() + _, _, err = tests.DeleteFiles(preDeleteSpec2, serverDetails) + assert.NoError(t, err) + + preDeleteSpec3 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a3.in").BuildSpec() + _, _, err = tests.DeleteFiles(preDeleteSpec3, serverDetails) + assert.NoError(t, err) + + t.Logf("Pre-deleted %d files to simulate 404 scenario", filesToPreDelete) + + // Step 4: Now call DeleteFiles with the original reader + // The reader still contains paths to the pre-deleted files, so we'll get 404s + successCount, failCount, deleteErr := deleteCommand.DeleteFiles(pathsReader) + gofrogio.Close(pathsReader, &err) + + // Step 5: Verify the fix - counts should be properly returned even with 404 errors + // Before the fix: successCount=0, failCount=0 (bug!) + // After the fix: successCount + failCount should equal totalFiles + t.Logf("Delete result: success=%d, fail=%d, err=%v", successCount, failCount, deleteErr) + + // The total of success + fail should equal what we tried to delete + assert.Equal(t, totalFiles, successCount+failCount, + "Total of success + fail counts should equal the number of files we tried to delete") + + // We should have some successful deletes (files that weren't pre-deleted) + expectedSuccessful := totalFiles - filesToPreDelete + assert.Equal(t, expectedSuccessful, successCount, + "Success count should match files that still existed") + + // We should have some failures (the pre-deleted files that returned 404) + assert.Equal(t, filesToPreDelete, failCount, + "Fail count should match the pre-deleted files that returned 404") + + // Verify all files are now gone + reader, err = searchCmd.Search() + assert.NoError(t, err) + remainingFiles, err := reader.Length() + assert.NoError(t, err) + readerCloseAndAssert(t, reader) + assert.Equal(t, 0, remainingFiles, "All files should be deleted") + + // Cleanup + cleanArtifactoryTest() +} + func TestArtifactoryMultipleFileSpecsUpload(t *testing.T) { initArtifactoryTest(t, "") specFile, err := tests.CreateSpec(tests.UploadMultipleFileSpecs) diff --git a/go.mod b/go.mod index 9d2f7c0ca..737e4c7f0 100644 --- a/go.mod +++ b/go.mod @@ -285,7 +285,8 @@ replace github.com/gfleury/go-bitbucket-v1 => github.com/gfleury/go-bitbucket-v1 //replace github.com/jfrog/jfrog-cli-core/v2 => ../jfrog-cli-core // replace github.com/jfrog/jfrog-cli-artifactory => github.com/fluxxBot/jfrog-cli-artifactory v0.0.0-20260105073552-ae4f86048a11 -// +replace github.com/jfrog/jfrog-cli-artifactory => github.com/bhanurp/jfrog-cli-artifactory v0.1.12-0.20260108075108-b5389ca4d380 + //replace github.com/jfrog/build-info-go => github.com/fluxxBot/build-info-go v1.10.10-0.20260105070825-d3f36f619ba5 // //replace github.com/jfrog/jfrog-cli-core/v2 => github.com/fluxxBot/jfrog-cli-core/v2 v2.58.1-0.20260105065921-c6488910f44c diff --git a/go.sum b/go.sum index 00de23985..1432bdd10 100644 --- a/go.sum +++ b/go.sum @@ -729,6 +729,8 @@ github.com/beevik/etree v1.6.0 h1:u8Kwy8pp9D9XeITj2Z0XtA5qqZEmtJtuXZRQi+j03eE= github.com/beevik/etree v1.6.0/go.mod h1:bh4zJxiIr62SOf9pRzN7UUYaEDa9HEKafK25+sLc0Gc= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= +github.com/bhanurp/jfrog-cli-artifactory v0.1.12-0.20260108075108-b5389ca4d380 h1:cj4idl/pC+WMSqj5jddYgrdFfaoWudzpjn0/Zt7ba8U= +github.com/bhanurp/jfrog-cli-artifactory v0.1.12-0.20260108075108-b5389ca4d380/go.mod h1:U/1q7jEO0YGSAWZEZiEmo0lZHI48xBorsFuL/F8C1fU= github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ= github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk= github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= @@ -1216,8 +1218,6 @@ github.com/jfrog/jfrog-apps-config v1.0.1 h1:mtv6k7g8A8BVhlHGlSveapqf4mJfonwvXYL github.com/jfrog/jfrog-apps-config v1.0.1/go.mod h1:8AIIr1oY9JuH5dylz2S6f8Ym2MaadPLR6noCBO4C22w= github.com/jfrog/jfrog-cli-application v1.0.2-0.20251231144110-a68c3ac11c7a h1:XoJ3w2AFi7zniimALNK3idw9bzY9MwB/FM45TMgxYAY= github.com/jfrog/jfrog-cli-application v1.0.2-0.20251231144110-a68c3ac11c7a/go.mod h1:xum2HquWO5uExa/A7MQs3TgJJVEeoqTR+6Z4mfBr1Xw= -github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260107090044-56a45e5c560e h1:+qB6eWbzeSOh5i6Pc0sC9arG8r5f6GLZm722jDyQ6nI= -github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260107090044-56a45e5c560e/go.mod h1:U/1q7jEO0YGSAWZEZiEmo0lZHI48xBorsFuL/F8C1fU= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260106204841-744f3f71817b h1:gGGmYXuYvcNns1BnLQI13lC+pgMxrmenx+ramtolQuA= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260106204841-744f3f71817b/go.mod h1:+Hnaikp/xCSPD/q7txxRy4Zc0wzjW/usrCSf+6uONSQ= github.com/jfrog/jfrog-cli-evidence v0.8.3-0.20251225153025-9d8ac181d615 h1:y5an0bojHL00ipHP1QuBUrVcP+XK+yZHHOJ/r1I0RUM= From 5ba3fb01ace4c4d4963cca1956c755d338ac9ed6 Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Fri, 9 Jan 2026 14:46:16 +0530 Subject: [PATCH 2/9] Added e2e for DeleteCommand returns 0 for SuccessCount and FailCount when errors occur --- artifactory_test.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index 6de9f54e1..026bcd197 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2846,21 +2846,25 @@ func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { readerLength, err := pathsReader.Length() assert.NoError(t, err) assert.Equal(t, totalFiles, readerLength, "Reader should contain all uploaded files") + t.Logf("PathsReader contains %d items (expected %d)", readerLength, totalFiles) + + // Reset the reader after Length() consumed it - required before passing to DeleteFiles + pathsReader.Reset() // Step 3: Simulate race condition - delete some files directly BEFORE calling DeleteFiles // This will cause 404 errors when DeleteFiles tries to delete them filesToPreDelete := 3 preDeleteSpec1 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a1.in").BuildSpec() _, _, err = tests.DeleteFiles(preDeleteSpec1, serverDetails) - assert.NoError(t, err) + assert.NoError(t, err, "Failed to pre-delete a1.in") preDeleteSpec2 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a2.in").BuildSpec() _, _, err = tests.DeleteFiles(preDeleteSpec2, serverDetails) - assert.NoError(t, err) + assert.NoError(t, err, "Failed to pre-delete a2.in") preDeleteSpec3 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a3.in").BuildSpec() _, _, err = tests.DeleteFiles(preDeleteSpec3, serverDetails) - assert.NoError(t, err) + assert.NoError(t, err, "Failed to pre-delete a3.in") t.Logf("Pre-deleted %d files to simulate 404 scenario", filesToPreDelete) @@ -2874,18 +2878,24 @@ func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { // After the fix: successCount + failCount should equal totalFiles t.Logf("Delete result: success=%d, fail=%d, err=%v", successCount, failCount, deleteErr) + // Calculate expected values for clearer error messages + expectedSuccessful := totalFiles - filesToPreDelete + totalProcessed := successCount + failCount + // The total of success + fail should equal what we tried to delete - assert.Equal(t, totalFiles, successCount+failCount, - "Total of success + fail counts should equal the number of files we tried to delete") + assert.Equal(t, totalFiles, totalProcessed, + "Total processed (success=%d + fail=%d = %d) should equal totalFiles=%d", + successCount, failCount, totalProcessed, totalFiles) // We should have some successful deletes (files that weren't pre-deleted) - expectedSuccessful := totalFiles - filesToPreDelete assert.Equal(t, expectedSuccessful, successCount, - "Success count should match files that still existed") + "Success count=%d should match expected=%d (totalFiles=%d - preDeleted=%d)", + successCount, expectedSuccessful, totalFiles, filesToPreDelete) // We should have some failures (the pre-deleted files that returned 404) assert.Equal(t, filesToPreDelete, failCount, - "Fail count should match the pre-deleted files that returned 404") + "Fail count=%d should match pre-deleted files=%d", + failCount, filesToPreDelete) // Verify all files are now gone reader, err = searchCmd.Search() From 7c60385cc640b415b0069f2bfef6342170decf5e Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Fri, 9 Jan 2026 15:03:37 +0530 Subject: [PATCH 3/9] Added e2e for DeleteCommand returns 0 for SuccessCount and FailCount when errors occur --- artifactory_test.go | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index 026bcd197..7a9f9111f 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2855,18 +2855,38 @@ func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { // This will cause 404 errors when DeleteFiles tries to delete them filesToPreDelete := 3 preDeleteSpec1 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a1.in").BuildSpec() - _, _, err = tests.DeleteFiles(preDeleteSpec1, serverDetails) + preDeleteSuccess1, preDeleteFail1, err := tests.DeleteFiles(preDeleteSpec1, serverDetails) assert.NoError(t, err, "Failed to pre-delete a1.in") + t.Logf("Pre-delete a1.in: success=%d, fail=%d", preDeleteSuccess1, preDeleteFail1) preDeleteSpec2 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a2.in").BuildSpec() - _, _, err = tests.DeleteFiles(preDeleteSpec2, serverDetails) + preDeleteSuccess2, preDeleteFail2, err := tests.DeleteFiles(preDeleteSpec2, serverDetails) assert.NoError(t, err, "Failed to pre-delete a2.in") + t.Logf("Pre-delete a2.in: success=%d, fail=%d", preDeleteSuccess2, preDeleteFail2) preDeleteSpec3 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a3.in").BuildSpec() - _, _, err = tests.DeleteFiles(preDeleteSpec3, serverDetails) + preDeleteSuccess3, preDeleteFail3, err := tests.DeleteFiles(preDeleteSpec3, serverDetails) assert.NoError(t, err, "Failed to pre-delete a3.in") + t.Logf("Pre-delete a3.in: success=%d, fail=%d", preDeleteSuccess3, preDeleteFail3) - t.Logf("Pre-deleted %d files to simulate 404 scenario", filesToPreDelete) + // Verify pre-deletion actually removed the files + totalPreDeleted := preDeleteSuccess1 + preDeleteSuccess2 + preDeleteSuccess3 + assert.Equal(t, filesToPreDelete, totalPreDeleted, + "Pre-deletion should have deleted %d files, but deleted %d", filesToPreDelete, totalPreDeleted) + + // Double-check by searching - should now have (totalFiles - filesToPreDelete) files + reader, err = searchCmd.Search() + assert.NoError(t, err, "Failed to search after pre-deletion") + filesAfterPreDelete, err := reader.Length() + assert.NoError(t, err) + readerCloseAndAssert(t, reader) + expectedAfterPreDelete := totalFiles - filesToPreDelete + assert.Equal(t, expectedAfterPreDelete, filesAfterPreDelete, + "After pre-deleting %d files, should have %d remaining, but found %d", + filesToPreDelete, expectedAfterPreDelete, filesAfterPreDelete) + + t.Logf("Pre-deleted %d files, verified %d files remaining (expected %d)", + totalPreDeleted, filesAfterPreDelete, expectedAfterPreDelete) // Step 4: Now call DeleteFiles with the original reader // The reader still contains paths to the pre-deleted files, so we'll get 404s From af7b1ba63652efe29e2122d96af7f9059fbb9bb4 Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Mon, 12 Jan 2026 16:10:04 +0530 Subject: [PATCH 4/9] Added partial case and complete delete case --- artifactory_test.go | 205 +++++++++++++++++++++++++++++--------------- 1 file changed, 134 insertions(+), 71 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index 7a9f9111f..411dd448e 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2801,18 +2801,16 @@ func TestArtifactoryDeleteByProps(t *testing.T) { cleanArtifactoryTest() } -// TestArtifactoryDeleteCountsWithPartial404 tests that delete returns accurate -// success/fail counts when 404 errors occur due to files being deleted between -// GetPathsToDelete and DeleteFiles (race condition scenario). -func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { +// TestArtifactoryDeleteCountsWithFull404 tests that delete returns accurate +// counts when ALL files are already deleted (all 404 errors). +func TestArtifactoryDeleteCountsWithFull404(t *testing.T) { initArtifactoryTest(t, "") - // Step 1: Upload more than 5 test files (using testdata/a/ which has 9 .in files) + // Step 1: Upload test files runRt(t, "upload", "testdata/a/*.in", tests.RtRepo1+"/delete-404-test/", "--flat=true") - runRt(t, "upload", "testdata/a/b/*.in", tests.RtRepo1+"/delete-404-test/", "--flat=true") - runRt(t, "upload", "testdata/a/b/c/*.in", tests.RtRepo1+"/delete-404-test/", "--flat=true") + t.Log("Uploaded test files") - // Verify we have more than 5 files uploaded + // Step 2: Search to find all uploaded files searchSpec := spec.NewBuilder(). Pattern(tests.RtRepo1 + "/delete-404-test/*.in"). BuildSpec() @@ -2823,10 +2821,10 @@ func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { totalFiles, err := reader.Length() assert.NoError(t, err) readerCloseAndAssert(t, reader) - assert.Greater(t, totalFiles, 5, "Should have uploaded more than 5 files for this test") - t.Logf("Uploaded %d files for delete-404 test", totalFiles) + assert.Greater(t, totalFiles, 0, "Should have uploaded files for this test") + t.Logf("Found %d files to delete", totalFiles) - // Step 2: Create delete command and get paths to delete + // Step 3: Create delete command and get paths to delete deleteSpec := spec.NewBuilder(). Pattern(tests.RtRepo1 + "/delete-404-test/*.in"). BuildSpec() @@ -2838,92 +2836,157 @@ func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { SetDryRun(false). SetQuiet(true) - // Get paths to delete (this queries Artifactory for matching files) + // Get paths to delete pathsReader, err := deleteCommand.GetPathsToDelete() assert.NoError(t, err) - // Verify reader has the expected number of items + // Verify reader has the expected files readerLength, err := pathsReader.Length() assert.NoError(t, err) assert.Equal(t, totalFiles, readerLength, "Reader should contain all uploaded files") - t.Logf("PathsReader contains %d items (expected %d)", readerLength, totalFiles) - - // Reset the reader after Length() consumed it - required before passing to DeleteFiles + t.Logf("PathsReader contains %d items", readerLength) pathsReader.Reset() - // Step 3: Simulate race condition - delete some files directly BEFORE calling DeleteFiles - // This will cause 404 errors when DeleteFiles tries to delete them - filesToPreDelete := 3 - preDeleteSpec1 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a1.in").BuildSpec() - preDeleteSuccess1, preDeleteFail1, err := tests.DeleteFiles(preDeleteSpec1, serverDetails) - assert.NoError(t, err, "Failed to pre-delete a1.in") - t.Logf("Pre-delete a1.in: success=%d, fail=%d", preDeleteSuccess1, preDeleteFail1) - - preDeleteSpec2 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a2.in").BuildSpec() - preDeleteSuccess2, preDeleteFail2, err := tests.DeleteFiles(preDeleteSpec2, serverDetails) - assert.NoError(t, err, "Failed to pre-delete a2.in") - t.Logf("Pre-delete a2.in: success=%d, fail=%d", preDeleteSuccess2, preDeleteFail2) - - preDeleteSpec3 := spec.NewBuilder().Pattern(tests.RtRepo1 + "/delete-404-test/a3.in").BuildSpec() - preDeleteSuccess3, preDeleteFail3, err := tests.DeleteFiles(preDeleteSpec3, serverDetails) - assert.NoError(t, err, "Failed to pre-delete a3.in") - t.Logf("Pre-delete a3.in: success=%d, fail=%d", preDeleteSuccess3, preDeleteFail3) - - // Verify pre-deletion actually removed the files - totalPreDeleted := preDeleteSuccess1 + preDeleteSuccess2 + preDeleteSuccess3 - assert.Equal(t, filesToPreDelete, totalPreDeleted, - "Pre-deletion should have deleted %d files, but deleted %d", filesToPreDelete, totalPreDeleted) - - // Double-check by searching - should now have (totalFiles - filesToPreDelete) files + // Step 4: Delete all files (first delete - should all succeed) + successCount1, failCount1, deleteErr1 := deleteCommand.DeleteFiles(pathsReader) + t.Logf("First delete: success=%d, fail=%d, err=%v", successCount1, failCount1, deleteErr1) + + assert.NoError(t, deleteErr1, "First delete should succeed without errors") + assert.Equal(t, totalFiles, successCount1, "First delete should succeed for all %d files", totalFiles) + assert.Equal(t, 0, failCount1, "First delete should have no failures") + + // Step 5: Verify all files are now gone reader, err = searchCmd.Search() - assert.NoError(t, err, "Failed to search after pre-deletion") - filesAfterPreDelete, err := reader.Length() + assert.NoError(t, err) + remainingFiles, err := reader.Length() + assert.NoError(t, err) + readerCloseAndAssert(t, reader) + assert.Equal(t, 0, remainingFiles, "All files should be deleted after first delete") + t.Log("Verified all files are deleted") + + // Step 6: Reset the reader and try to delete AGAIN (all should get 404) + pathsReader.Reset() + successCount2, failCount2, deleteErr2 := deleteCommand.DeleteFiles(pathsReader) + gofrogio.Close(pathsReader, &err) + + t.Logf("Second delete (all 404): success=%d, fail=%d, err=%v", successCount2, failCount2, deleteErr2) + + // Step 7: Verify the results - all should fail with 404 + totalProcessed := successCount2 + failCount2 + assert.Equal(t, totalFiles, totalProcessed, + "Total processed (success=%d + fail=%d = %d) should equal totalFiles=%d", + successCount2, failCount2, totalProcessed, totalFiles) + + assert.Equal(t, 0, successCount2, + "Second delete should have 0 successes (files already deleted), got %d", successCount2) + + assert.Equal(t, totalFiles, failCount2, + "Second delete should have %d failures (all 404), got %d", totalFiles, failCount2) + + // Cleanup + cleanArtifactoryTest() +} + +// TestArtifactoryDeleteCountsWithPartial404 tests that delete returns accurate +// counts when SOME files are already deleted (partial 404 errors). +func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { + initArtifactoryTest(t, "") + + // Step 1: Upload 5 specific test files + runRt(t, "upload", "testdata/a/a1.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") + runRt(t, "upload", "testdata/a/a2.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") + runRt(t, "upload", "testdata/a/a3.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") + runRt(t, "upload", "testdata/a/b/b1.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") + runRt(t, "upload", "testdata/a/b/b2.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") + t.Log("Uploaded 5 test files") + + // Step 2: Verify we have 5 files + searchSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-partial-404/*.in"). + BuildSpec() + searchCmd := generic.NewSearchCommand() + searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) + reader, err := searchCmd.Search() + assert.NoError(t, err) + totalFiles, err := reader.Length() assert.NoError(t, err) readerCloseAndAssert(t, reader) - expectedAfterPreDelete := totalFiles - filesToPreDelete - assert.Equal(t, expectedAfterPreDelete, filesAfterPreDelete, - "After pre-deleting %d files, should have %d remaining, but found %d", - filesToPreDelete, expectedAfterPreDelete, filesAfterPreDelete) + assert.Equal(t, 5, totalFiles, "Should have uploaded 5 files") + t.Logf("Verified %d files uploaded", totalFiles) + + // Step 3: Create delete command and get paths for ALL 5 files + deleteSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-partial-404/*.in"). + BuildSpec() - t.Logf("Pre-deleted %d files, verified %d files remaining (expected %d)", - totalPreDeleted, filesAfterPreDelete, expectedAfterPreDelete) + deleteCommand := generic.NewDeleteCommand() + deleteCommand.SetThreads(1). + SetSpec(deleteSpec). + SetServerDetails(serverDetails). + SetDryRun(false). + SetQuiet(true) + + // Get paths for all 5 files + pathsReader, err := deleteCommand.GetPathsToDelete() + assert.NoError(t, err) + readerLength, err := pathsReader.Length() + assert.NoError(t, err) + assert.Equal(t, 5, readerLength, "Reader should contain 5 files") + t.Logf("PathsReader contains %d items", readerLength) + pathsReader.Reset() - // Step 4: Now call DeleteFiles with the original reader - // The reader still contains paths to the pre-deleted files, so we'll get 404s + // Step 4: Delete only 2 files (a1.in and a2.in) + preDeleteSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-partial-404/a1.in"). + BuildSpec() + preDeleteSuccess1, _, err := tests.DeleteFiles(preDeleteSpec, serverDetails) + assert.NoError(t, err) + assert.Equal(t, 1, preDeleteSuccess1, "Should have deleted a1.in") + t.Log("Pre-deleted a1.in") + + preDeleteSpec2 := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-partial-404/a2.in"). + BuildSpec() + preDeleteSuccess2, _, err := tests.DeleteFiles(preDeleteSpec2, serverDetails) + assert.NoError(t, err) + assert.Equal(t, 1, preDeleteSuccess2, "Should have deleted a2.in") + t.Log("Pre-deleted a2.in") + + // Step 5: Verify only 3 files remain + reader, err = searchCmd.Search() + assert.NoError(t, err) + remainingFiles, err := reader.Length() + assert.NoError(t, err) + readerCloseAndAssert(t, reader) + assert.Equal(t, 3, remainingFiles, "Should have 3 files remaining after pre-delete") + t.Logf("Verified %d files remaining after pre-deleting 2", remainingFiles) + + // Step 6: Now try to delete ALL 5 files using original pathsReader + // Expected: 3 success (a3, b1, b2), 2 fail (a1, a2 already deleted = 404) successCount, failCount, deleteErr := deleteCommand.DeleteFiles(pathsReader) gofrogio.Close(pathsReader, &err) - // Step 5: Verify the fix - counts should be properly returned even with 404 errors - // Before the fix: successCount=0, failCount=0 (bug!) - // After the fix: successCount + failCount should equal totalFiles t.Logf("Delete result: success=%d, fail=%d, err=%v", successCount, failCount, deleteErr) - // Calculate expected values for clearer error messages - expectedSuccessful := totalFiles - filesToPreDelete + // Step 7: Verify counts totalProcessed := successCount + failCount + assert.Equal(t, 5, totalProcessed, + "Total processed (success=%d + fail=%d = %d) should equal 5", + successCount, failCount, totalProcessed) - // The total of success + fail should equal what we tried to delete - assert.Equal(t, totalFiles, totalProcessed, - "Total processed (success=%d + fail=%d = %d) should equal totalFiles=%d", - successCount, failCount, totalProcessed, totalFiles) + assert.Equal(t, 3, successCount, + "Should have 3 successes (a3, b1, b2 still existed), got %d", successCount) - // We should have some successful deletes (files that weren't pre-deleted) - assert.Equal(t, expectedSuccessful, successCount, - "Success count=%d should match expected=%d (totalFiles=%d - preDeleted=%d)", - successCount, expectedSuccessful, totalFiles, filesToPreDelete) + assert.Equal(t, 2, failCount, + "Should have 2 failures (a1, a2 already deleted = 404), got %d", failCount) - // We should have some failures (the pre-deleted files that returned 404) - assert.Equal(t, filesToPreDelete, failCount, - "Fail count=%d should match pre-deleted files=%d", - failCount, filesToPreDelete) - - // Verify all files are now gone + // Step 8: Verify all files are now gone reader, err = searchCmd.Search() assert.NoError(t, err) - remainingFiles, err := reader.Length() + finalCount, err := reader.Length() assert.NoError(t, err) readerCloseAndAssert(t, reader) - assert.Equal(t, 0, remainingFiles, "All files should be deleted") + assert.Equal(t, 0, finalCount, "All files should be deleted") // Cleanup cleanArtifactoryTest() From 2e9a7b0bcbb62f31287213253fd496869f257bc5 Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Mon, 12 Jan 2026 18:01:35 +0530 Subject: [PATCH 5/9] Removed wildcard patterns in deletion --- artifactory_test.go | 251 ++++++++++++++++++-------------------------- 1 file changed, 102 insertions(+), 149 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index 411dd448e..ec1d2aec4 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2806,82 +2806,58 @@ func TestArtifactoryDeleteByProps(t *testing.T) { func TestArtifactoryDeleteCountsWithFull404(t *testing.T) { initArtifactoryTest(t, "") - // Step 1: Upload test files - runRt(t, "upload", "testdata/a/*.in", tests.RtRepo1+"/delete-404-test/", "--flat=true") - t.Log("Uploaded test files") - - // Step 2: Search to find all uploaded files + // Step 1: Upload 3 specific test files by explicit name + testFiles := []string{"a1.in", "a2.in", "a3.in"} + for _, f := range testFiles { + runRt(t, "upload", "testdata/a/"+f, tests.RtRepo1+"/delete-404-full/"+f) + } + t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) + + // Step 2: Delete each file explicitly by name and verify success + for _, f := range testFiles { + deleteSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-404-full/" + f). + BuildSpec() + success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) + assert.NoError(t, err, "First delete of %s should succeed", f) + assert.Equal(t, 1, success, "First delete of %s should have 1 success", f) + assert.Equal(t, 0, fail, "First delete of %s should have 0 failures", f) + } + t.Log("First delete: all files deleted successfully") + + // Step 3: Verify all files are now gone searchSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-404-test/*.in"). + Pattern(tests.RtRepo1 + "/delete-404-full/*"). BuildSpec() searchCmd := generic.NewSearchCommand() searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) reader, err := searchCmd.Search() assert.NoError(t, err) - totalFiles, err := reader.Length() - assert.NoError(t, err) - readerCloseAndAssert(t, reader) - assert.Greater(t, totalFiles, 0, "Should have uploaded files for this test") - t.Logf("Found %d files to delete", totalFiles) - - // Step 3: Create delete command and get paths to delete - deleteSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-404-test/*.in"). - BuildSpec() - - deleteCommand := generic.NewDeleteCommand() - deleteCommand.SetThreads(1). - SetSpec(deleteSpec). - SetServerDetails(serverDetails). - SetDryRun(false). - SetQuiet(true) - - // Get paths to delete - pathsReader, err := deleteCommand.GetPathsToDelete() - assert.NoError(t, err) - - // Verify reader has the expected files - readerLength, err := pathsReader.Length() - assert.NoError(t, err) - assert.Equal(t, totalFiles, readerLength, "Reader should contain all uploaded files") - t.Logf("PathsReader contains %d items", readerLength) - pathsReader.Reset() - - // Step 4: Delete all files (first delete - should all succeed) - successCount1, failCount1, deleteErr1 := deleteCommand.DeleteFiles(pathsReader) - t.Logf("First delete: success=%d, fail=%d, err=%v", successCount1, failCount1, deleteErr1) - - assert.NoError(t, deleteErr1, "First delete should succeed without errors") - assert.Equal(t, totalFiles, successCount1, "First delete should succeed for all %d files", totalFiles) - assert.Equal(t, 0, failCount1, "First delete should have no failures") - - // Step 5: Verify all files are now gone - reader, err = searchCmd.Search() - assert.NoError(t, err) remainingFiles, err := reader.Length() assert.NoError(t, err) readerCloseAndAssert(t, reader) - assert.Equal(t, 0, remainingFiles, "All files should be deleted after first delete") + assert.Equal(t, 0, remainingFiles, "All files should be deleted") t.Log("Verified all files are deleted") - // Step 6: Reset the reader and try to delete AGAIN (all should get 404) - pathsReader.Reset() - successCount2, failCount2, deleteErr2 := deleteCommand.DeleteFiles(pathsReader) - gofrogio.Close(pathsReader, &err) - - t.Logf("Second delete (all 404): success=%d, fail=%d, err=%v", successCount2, failCount2, deleteErr2) - - // Step 7: Verify the results - all should fail with 404 - totalProcessed := successCount2 + failCount2 - assert.Equal(t, totalFiles, totalProcessed, - "Total processed (success=%d + fail=%d = %d) should equal totalFiles=%d", - successCount2, failCount2, totalProcessed, totalFiles) - - assert.Equal(t, 0, successCount2, - "Second delete should have 0 successes (files already deleted), got %d", successCount2) - - assert.Equal(t, totalFiles, failCount2, - "Second delete should have %d failures (all 404), got %d", totalFiles, failCount2) + // Step 4: Try to delete each file AGAIN by explicit name (all should get 404) + totalSuccess := 0 + totalFail := 0 + for _, f := range testFiles { + deleteSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-404-full/" + f). + BuildSpec() + success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) + t.Logf("Second delete of %s: success=%d, fail=%d, err=%v", f, success, fail, err) + totalSuccess += success + totalFail += fail + } + t.Logf("Second delete totals: success=%d, fail=%d", totalSuccess, totalFail) + + // Step 5: Verify the results - all should fail with 404 + assert.Equal(t, 0, totalSuccess, + "Second delete should have 0 successes (files already deleted), got %d", totalSuccess) + assert.Equal(t, len(testFiles), totalFail, + "Second delete should have %d failures (all 404), got %d", len(testFiles), totalFail) // Cleanup cleanArtifactoryTest() @@ -2892,97 +2868,74 @@ func TestArtifactoryDeleteCountsWithFull404(t *testing.T) { func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { initArtifactoryTest(t, "") - // Step 1: Upload 5 specific test files - runRt(t, "upload", "testdata/a/a1.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") - runRt(t, "upload", "testdata/a/a2.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") - runRt(t, "upload", "testdata/a/a3.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") - runRt(t, "upload", "testdata/a/b/b1.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") - runRt(t, "upload", "testdata/a/b/b2.in", tests.RtRepo1+"/delete-partial-404/", "--flat=true") - t.Log("Uploaded 5 test files") + // Step 1: Upload 5 specific test files by explicit name + testFiles := []string{"a1.in", "a2.in", "a3.in", "b1.in", "b2.in"} + sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in", "testdata/a/b/b1.in", "testdata/a/b/b2.in"} + for i, f := range testFiles { + runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-partial-404/"+f) + } + t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) + + // Step 2: Pre-delete 2 files (a1.in and a2.in) and confirm deletion + filesToPreDelete := []string{"a1.in", "a2.in"} + for _, f := range filesToPreDelete { + deleteSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-partial-404/" + f). + BuildSpec() + success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) + assert.NoError(t, err, "Pre-delete of %s should succeed", f) + assert.Equal(t, 1, success, "Pre-delete of %s should have 1 success", f) + assert.Equal(t, 0, fail, "Pre-delete of %s should have 0 failures", f) + t.Logf("Pre-deleted %s successfully", f) + } + + // Step 3: Verify pre-deleted files are gone (search should return 404 or 0 results) + for _, f := range filesToPreDelete { + searchSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-partial-404/" + f). + BuildSpec() + searchCmd := generic.NewSearchCommand() + searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) + reader, err := searchCmd.Search() + assert.NoError(t, err) + count, err := reader.Length() + assert.NoError(t, err) + readerCloseAndAssert(t, reader) + assert.Equal(t, 0, count, "File %s should be deleted", f) + } + t.Log("Verified pre-deleted files are gone") - // Step 2: Verify we have 5 files + // Step 4: Now try to delete ALL 5 files by explicit name + // Expected: 3 success (a3, b1, b2), 2 fail (a1, a2 already deleted = 404) + totalSuccess := 0 + totalFail := 0 + for _, f := range testFiles { + deleteSpec := spec.NewBuilder(). + Pattern(tests.RtRepo1 + "/delete-partial-404/" + f). + BuildSpec() + success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) + t.Logf("Delete %s: success=%d, fail=%d, err=%v", f, success, fail, err) + totalSuccess += success + totalFail += fail + } + t.Logf("Delete all 5 files result: success=%d, fail=%d", totalSuccess, totalFail) + + // Step 5: Verify counts + // 3 files existed (a3, b1, b2) = 3 success + // 2 files already deleted (a1, a2) = 2 fail + assert.Equal(t, 3, totalSuccess, + "Should have 3 successes (a3, b1, b2 still existed), got %d", totalSuccess) + assert.Equal(t, 2, totalFail, + "Should have 2 failures (a1, a2 already deleted = 404), got %d", totalFail) + + // Step 6: Verify all files are now gone searchSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/*.in"). + Pattern(tests.RtRepo1 + "/delete-partial-404/*"). BuildSpec() searchCmd := generic.NewSearchCommand() searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) reader, err := searchCmd.Search() assert.NoError(t, err) - totalFiles, err := reader.Length() - assert.NoError(t, err) - readerCloseAndAssert(t, reader) - assert.Equal(t, 5, totalFiles, "Should have uploaded 5 files") - t.Logf("Verified %d files uploaded", totalFiles) - - // Step 3: Create delete command and get paths for ALL 5 files - deleteSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/*.in"). - BuildSpec() - - deleteCommand := generic.NewDeleteCommand() - deleteCommand.SetThreads(1). - SetSpec(deleteSpec). - SetServerDetails(serverDetails). - SetDryRun(false). - SetQuiet(true) - - // Get paths for all 5 files - pathsReader, err := deleteCommand.GetPathsToDelete() - assert.NoError(t, err) - readerLength, err := pathsReader.Length() - assert.NoError(t, err) - assert.Equal(t, 5, readerLength, "Reader should contain 5 files") - t.Logf("PathsReader contains %d items", readerLength) - pathsReader.Reset() - - // Step 4: Delete only 2 files (a1.in and a2.in) - preDeleteSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/a1.in"). - BuildSpec() - preDeleteSuccess1, _, err := tests.DeleteFiles(preDeleteSpec, serverDetails) - assert.NoError(t, err) - assert.Equal(t, 1, preDeleteSuccess1, "Should have deleted a1.in") - t.Log("Pre-deleted a1.in") - - preDeleteSpec2 := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/a2.in"). - BuildSpec() - preDeleteSuccess2, _, err := tests.DeleteFiles(preDeleteSpec2, serverDetails) - assert.NoError(t, err) - assert.Equal(t, 1, preDeleteSuccess2, "Should have deleted a2.in") - t.Log("Pre-deleted a2.in") - - // Step 5: Verify only 3 files remain - reader, err = searchCmd.Search() - assert.NoError(t, err) - remainingFiles, err := reader.Length() - assert.NoError(t, err) - readerCloseAndAssert(t, reader) - assert.Equal(t, 3, remainingFiles, "Should have 3 files remaining after pre-delete") - t.Logf("Verified %d files remaining after pre-deleting 2", remainingFiles) - - // Step 6: Now try to delete ALL 5 files using original pathsReader - // Expected: 3 success (a3, b1, b2), 2 fail (a1, a2 already deleted = 404) - successCount, failCount, deleteErr := deleteCommand.DeleteFiles(pathsReader) - gofrogio.Close(pathsReader, &err) - - t.Logf("Delete result: success=%d, fail=%d, err=%v", successCount, failCount, deleteErr) - - // Step 7: Verify counts - totalProcessed := successCount + failCount - assert.Equal(t, 5, totalProcessed, - "Total processed (success=%d + fail=%d = %d) should equal 5", - successCount, failCount, totalProcessed) - - assert.Equal(t, 3, successCount, - "Should have 3 successes (a3, b1, b2 still existed), got %d", successCount) - - assert.Equal(t, 2, failCount, - "Should have 2 failures (a1, a2 already deleted = 404), got %d", failCount) - - // Step 8: Verify all files are now gone - reader, err = searchCmd.Search() - assert.NoError(t, err) finalCount, err := reader.Length() assert.NoError(t, err) readerCloseAndAssert(t, reader) From 4eab4009b1d360e661d7096ef8e0902005af0ad1 Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Tue, 13 Jan 2026 03:19:54 +0530 Subject: [PATCH 6/9] Added e2e test scenarios for partial and full delete --- artifactory_test.go | 136 ++++++++++++++++++++++--------------------- utils/tests/utils.go | 109 ++++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+), 66 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index ec1d2aec4..899683f9d 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2801,33 +2801,31 @@ func TestArtifactoryDeleteByProps(t *testing.T) { cleanArtifactoryTest() } -// TestArtifactoryDeleteCountsWithFull404 tests that delete returns accurate -// counts when ALL files are already deleted (all 404 errors). -func TestArtifactoryDeleteCountsWithFull404(t *testing.T) { +// TestArtifactoryDeleteCountsNonExistentFiles tests that delete correctly handles +// files that don't exist. The CLI searches first, so non-existent files return +// All 3 files should return 404 on second delete attempt. +func TestArtifactoryDeleteCountsNonExistentFiles(t *testing.T) { initArtifactoryTest(t, "") // Step 1: Upload 3 specific test files by explicit name testFiles := []string{"a1.in", "a2.in", "a3.in"} for _, f := range testFiles { - runRt(t, "upload", "testdata/a/"+f, tests.RtRepo1+"/delete-404-full/"+f) + runRt(t, "upload", "testdata/a/"+f, tests.RtRepo1+"/delete-nonexistent/"+f) } t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) - // Step 2: Delete each file explicitly by name and verify success + // Step 2: Delete each file using direct DELETE API and verify success (HTTP 204) for _, f := range testFiles { - deleteSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-404-full/" + f). - BuildSpec() - success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) - assert.NoError(t, err, "First delete of %s should succeed", f) - assert.Equal(t, 1, success, "First delete of %s should have 1 success", f) - assert.Equal(t, 0, fail, "First delete of %s should have 0 failures", f) + artifactPath := tests.RtRepo1 + "/delete-nonexistent/" + f + success, err := tests.DeleteFileDirect(serverDetails, artifactPath) + assert.NoError(t, err, "First delete of %s should not return error", f) + assert.True(t, success, "First delete of %s should succeed (HTTP 204)", f) } - t.Log("First delete: all files deleted successfully") + t.Log("First delete: all files deleted successfully (HTTP 204)") - // Step 3: Verify all files are now gone + // Step 3: Verify all files are now gone via search searchSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-404-full/*"). + Pattern(tests.RtRepo1 + "/delete-nonexistent/*"). BuildSpec() searchCmd := generic.NewSearchCommand() searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) @@ -2839,60 +2837,63 @@ func TestArtifactoryDeleteCountsWithFull404(t *testing.T) { assert.Equal(t, 0, remainingFiles, "All files should be deleted") t.Log("Verified all files are deleted") - // Step 4: Try to delete each file AGAIN by explicit name (all should get 404) - totalSuccess := 0 - totalFail := 0 + // Step 4: Try to delete ALL files AGAIN using CLI's DeleteFiles API + // Build list of artifact paths + var artifactPaths []string for _, f := range testFiles { - deleteSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-404-full/" + f). - BuildSpec() - success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) - t.Logf("Second delete of %s: success=%d, fail=%d, err=%v", f, success, fail, err) - totalSuccess += success - totalFail += fail - } - t.Logf("Second delete totals: success=%d, fail=%d", totalSuccess, totalFail) - - // Step 5: Verify the results - all should fail with 404 - assert.Equal(t, 0, totalSuccess, - "Second delete should have 0 successes (files already deleted), got %d", totalSuccess) - assert.Equal(t, len(testFiles), totalFail, - "Second delete should have %d failures (all 404), got %d", len(testFiles), totalFail) + artifactPaths = append(artifactPaths, tests.RtRepo1+"/delete-nonexistent/"+f) + } + t.Logf("Attempting second delete of paths: %v", artifactPaths) + + // Use CLI's DeleteFiles API which should properly count 404s as failures + // Error IS expected because all files return 404 + totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(serverDetails, artifactPaths) + // Error is expected when files don't exist (404 responses) + t.Logf("Second delete result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err) + + // Step 5: Verify - all 3 files should return 404 (failedCount=3) + expectedSuccess := 0 + expectedFail := len(testFiles) + t.Logf("Expected: successCount=%d, failCount=%d", expectedSuccess, expectedFail) + t.Logf("Actual: successCount=%d, failCount=%d", totalSuccess, totalFail) + assert.Equal(t, expectedSuccess, totalSuccess, + "Second delete should have 0 successes (all files already deleted), got %d", totalSuccess) + assert.Equal(t, expectedFail, totalFail, + "Second delete should have %d failures (all 404), got %d", expectedFail, totalFail) // Cleanup cleanArtifactoryTest() } -// TestArtifactoryDeleteCountsWithPartial404 tests that delete returns accurate -// counts when SOME files are already deleted (partial 404 errors). -func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { +// TestArtifactoryDeleteCountsPartiallyDeleted tests that delete correctly handles +// a mix of existing and non-existing files using CLI's DeleteFiles API. +// - 3 existing files should return success +// - 2 pre-deleted files should return failure (404) +func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) { initArtifactoryTest(t, "") // Step 1: Upload 5 specific test files by explicit name testFiles := []string{"a1.in", "a2.in", "a3.in", "b1.in", "b2.in"} sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in", "testdata/a/b/b1.in", "testdata/a/b/b2.in"} for i, f := range testFiles { - runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-partial-404/"+f) + runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-partial/"+f) } t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) - // Step 2: Pre-delete 2 files (a1.in and a2.in) and confirm deletion + // Step 2: Pre-delete 2 files (a1.in and a2.in) using direct DELETE API filesToPreDelete := []string{"a1.in", "a2.in"} for _, f := range filesToPreDelete { - deleteSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/" + f). - BuildSpec() - success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) - assert.NoError(t, err, "Pre-delete of %s should succeed", f) - assert.Equal(t, 1, success, "Pre-delete of %s should have 1 success", f) - assert.Equal(t, 0, fail, "Pre-delete of %s should have 0 failures", f) - t.Logf("Pre-deleted %s successfully", f) + artifactPath := tests.RtRepo1 + "/delete-partial/" + f + success, err := tests.DeleteFileDirect(serverDetails, artifactPath) + assert.NoError(t, err, "Pre-delete of %s should not return error", f) + assert.True(t, success, "Pre-delete of %s should succeed (HTTP 204)", f) + t.Logf("Pre-deleted %s successfully (HTTP 204)", f) } - // Step 3: Verify pre-deleted files are gone (search should return 404 or 0 results) + // Step 3: Verify pre-deleted files are gone via search for _, f := range filesToPreDelete { searchSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/" + f). + Pattern(tests.RtRepo1 + "/delete-partial/" + f). BuildSpec() searchCmd := generic.NewSearchCommand() searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) @@ -2905,32 +2906,35 @@ func TestArtifactoryDeleteCountsWithPartial404(t *testing.T) { } t.Log("Verified pre-deleted files are gone") - // Step 4: Now try to delete ALL 5 files by explicit name - // Expected: 3 success (a3, b1, b2), 2 fail (a1, a2 already deleted = 404) - totalSuccess := 0 - totalFail := 0 + // Step 4: Now try to delete ALL 5 files using CLI's DeleteFiles API + // - 3 files exist (a3, b1, b2) → should return success + // - 2 files don't exist (a1, a2) → should return failure (404) + var artifactPaths []string for _, f := range testFiles { - deleteSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/" + f). - BuildSpec() - success, fail, err := tests.DeleteFiles(deleteSpec, serverDetails) - t.Logf("Delete %s: success=%d, fail=%d, err=%v", f, success, fail, err) - totalSuccess += success - totalFail += fail + artifactPaths = append(artifactPaths, tests.RtRepo1+"/delete-partial/"+f) } - t.Logf("Delete all 5 files result: success=%d, fail=%d", totalSuccess, totalFail) + t.Logf("Attempting to delete paths: %v", artifactPaths) + + // Error IS expected because some files return 404 + totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(serverDetails, artifactPaths) + // Error is expected when some files don't exist (404 responses) + t.Logf("Delete all 5 files result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err) // Step 5: Verify counts // 3 files existed (a3, b1, b2) = 3 success - // 2 files already deleted (a1, a2) = 2 fail - assert.Equal(t, 3, totalSuccess, - "Should have 3 successes (a3, b1, b2 still existed), got %d", totalSuccess) - assert.Equal(t, 2, totalFail, - "Should have 2 failures (a1, a2 already deleted = 404), got %d", totalFail) + // 2 files didn't exist (a1, a2) = 2 fail (404) + expectedSuccess := len(testFiles) - len(filesToPreDelete) + expectedFail := len(filesToPreDelete) + t.Logf("Expected: successCount=%d, failCount=%d", expectedSuccess, expectedFail) + t.Logf("Actual: successCount=%d, failCount=%d", totalSuccess, totalFail) + assert.Equal(t, expectedSuccess, totalSuccess, + "Should have %d successes (a3, b1, b2 still existed), got %d", expectedSuccess, totalSuccess) + assert.Equal(t, expectedFail, totalFail, + "Should have %d failures (a1, a2 already deleted = 404), got %d", expectedFail, totalFail) // Step 6: Verify all files are now gone searchSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial-404/*"). + Pattern(tests.RtRepo1 + "/delete-partial/*"). BuildSpec() searchCmd := generic.NewSearchCommand() searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) diff --git a/utils/tests/utils.go b/utils/tests/utils.go index 2245a5562..b277d706e 100644 --- a/utils/tests/utils.go +++ b/utils/tests/utils.go @@ -35,8 +35,11 @@ import ( "github.com/jfrog/jfrog-cli/utils/summary" "github.com/jfrog/jfrog-client-go/artifactory/services" "github.com/jfrog/jfrog-client-go/artifactory/services/utils" + serviceutils "github.com/jfrog/jfrog-client-go/artifactory/services/utils" "github.com/jfrog/jfrog-client-go/auth" + "github.com/jfrog/jfrog-client-go/http/jfroghttpclient" clientutils "github.com/jfrog/jfrog-client-go/utils" + "github.com/jfrog/jfrog-client-go/utils/io/content" "github.com/jfrog/jfrog-client-go/utils/io/fileutils" "github.com/jfrog/jfrog-client-go/utils/log" "github.com/stretchr/testify/assert" @@ -237,6 +240,112 @@ func DeleteFiles(deleteSpec *spec.SpecFiles, serverDetails *config.ServerDetails return deleteCommand.DeleteFiles(reader) } +// DeleteFileDirect deletes a single file by its full Artifactory path (e.g., "repo/path/file.txt") +// This calls the DELETE API directly WITHOUT searching first. +// Returns: +// - success=true, err=nil: File was deleted (HTTP 204) +// - success=false, err=nil: File not found (HTTP 404) +// - success=false, err!=nil: Other error occurred +func DeleteFileDirect(serverDetails *config.ServerDetails, artifactoryPath string) (success bool, err error) { + // Create auth config + artAuth, err := serverDetails.CreateArtAuthConfig() + if err != nil { + return false, err + } + + // Build the full delete URL + deleteUrl := clientutils.AddTrailingSlashIfNeeded(artAuth.GetUrl()) + artifactoryPath + + // Create HTTP client + client, err := jfroghttpclient.JfrogClientBuilder(). + SetInsecureTls(serverDetails.InsecureTls). + AppendPreRequestInterceptor(artAuth.RunPreRequestFunctions). + Build() + if err != nil { + return false, err + } + + // Create HTTP client details with auth + httpClientsDetails := artAuth.CreateHttpClientDetails() + + // Send DELETE request directly + resp, body, err := client.SendDelete(deleteUrl, nil, &httpClientsDetails) + if err != nil { + return false, err + } + + // Check response status + if resp.StatusCode == 204 { + // Successfully deleted + return true, nil + } else if resp.StatusCode == 404 { + // File not found + return false, nil + } else { + // Other error + return false, fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body)) + } +} + +// DeleteFilesByPathsUsingCli deletes files by their explicit paths using the CLI's DeleteCommand.DeleteFiles API. +// This bypasses the search phase and directly calls delete on the provided paths. +// Returns successCount, failedCount, err - where failedCount includes 404 errors. +func DeleteFilesByPathsUsingCli(serverDetails *config.ServerDetails, artifactoryPaths []string) (successCount, failedCount int, err error) { + // Create a ContentWriter to build a reader with the file paths + writer, err := content.NewContentWriter(content.DefaultKey, true, false) + if err != nil { + return 0, 0, err + } + + // Write each path as a ResultItem + for _, artifactPath := range artifactoryPaths { + // Parse the path into repo/path/name + parts := strings.SplitN(artifactPath, "/", 2) + repo := parts[0] + pathAndName := "" + name := "" + if len(parts) > 1 { + pathAndName = parts[1] + // Split into path and name + lastSlash := strings.LastIndex(pathAndName, "/") + if lastSlash >= 0 { + pathAndName = pathAndName[:lastSlash] + name = artifactPath[strings.LastIndex(artifactPath, "/")+1:] + } else { + name = pathAndName + pathAndName = "." + } + } + item := serviceutils.ResultItem{ + Repo: repo, + Path: pathAndName, + Name: name, + Type: "file", + } + writer.Write(item) + } + + err = writer.Close() + if err != nil { + return 0, 0, err + } + + // Create a reader from the writer's file + reader := content.NewContentReader(writer.GetFilePath(), content.DefaultKey) + defer func() { + closeErr := reader.Close() + if err == nil { + err = closeErr + } + }() + + // Create DeleteCommand and call DeleteFiles + deleteCmd := generic.NewDeleteCommand() + deleteCmd.SetServerDetails(serverDetails) + + return deleteCmd.DeleteFiles(reader) +} + // This function makes no assertion, caller is responsible to assert as needed. func GetBuildInfo(serverDetails *config.ServerDetails, buildName, buildNumber string) (pbi *buildinfo.PublishedBuildInfo, found bool, err error) { servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false) From 37ec50fb6fc147c687cc376b89ceede166c1566f Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Tue, 13 Jan 2026 03:39:45 +0530 Subject: [PATCH 7/9] Updated files to be present in different sub folders --- artifactory_test.go | 37 +++++++++++++++++++++++-------------- utils/tests/utils.go | 11 ++++++----- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index 899683f9d..d8f117530 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2804,13 +2804,18 @@ func TestArtifactoryDeleteByProps(t *testing.T) { // TestArtifactoryDeleteCountsNonExistentFiles tests that delete correctly handles // files that don't exist. The CLI searches first, so non-existent files return // All 3 files should return 404 on second delete attempt. +// TestArtifactoryDeleteCountsNonExistentFiles tests that delete correctly counts +// 404 failures when all files have already been deleted. +// Files are placed in separate subfolders to prevent path reduction optimization. func TestArtifactoryDeleteCountsNonExistentFiles(t *testing.T) { initArtifactoryTest(t, "") - // Step 1: Upload 3 specific test files by explicit name - testFiles := []string{"a1.in", "a2.in", "a3.in"} - for _, f := range testFiles { - runRt(t, "upload", "testdata/a/"+f, tests.RtRepo1+"/delete-nonexistent/"+f) + // Step 1: Upload 3 specific test files in SEPARATE subfolders to prevent path reduction + // Each file goes in its own subfolder so delete service can't optimize to folder delete + testFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in"} + sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in"} + for i, f := range testFiles { + runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-nonexistent/"+f) } t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) @@ -2826,6 +2831,7 @@ func TestArtifactoryDeleteCountsNonExistentFiles(t *testing.T) { // Step 3: Verify all files are now gone via search searchSpec := spec.NewBuilder(). Pattern(tests.RtRepo1 + "/delete-nonexistent/*"). + Recursive(true). BuildSpec() searchCmd := generic.NewSearchCommand() searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) @@ -2869,19 +2875,21 @@ func TestArtifactoryDeleteCountsNonExistentFiles(t *testing.T) { // a mix of existing and non-existing files using CLI's DeleteFiles API. // - 3 existing files should return success // - 2 pre-deleted files should return failure (404) +// Files are placed in separate subfolders to prevent path reduction optimization. func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) { initArtifactoryTest(t, "") - // Step 1: Upload 5 specific test files by explicit name - testFiles := []string{"a1.in", "a2.in", "a3.in", "b1.in", "b2.in"} + // Step 1: Upload 5 specific test files in SEPARATE subfolders to prevent path reduction + // Each file goes in its own subfolder so delete service can't optimize to folder delete + testFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in", "f4/b1.in", "f5/b2.in"} sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in", "testdata/a/b/b1.in", "testdata/a/b/b2.in"} for i, f := range testFiles { runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-partial/"+f) } t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) - // Step 2: Pre-delete 2 files (a1.in and a2.in) using direct DELETE API - filesToPreDelete := []string{"a1.in", "a2.in"} + // Step 2: Pre-delete 2 files (f1/a1.in and f2/a2.in) using direct DELETE API + filesToPreDelete := []string{"f1/a1.in", "f2/a2.in"} for _, f := range filesToPreDelete { artifactPath := tests.RtRepo1 + "/delete-partial/" + f success, err := tests.DeleteFileDirect(serverDetails, artifactPath) @@ -2907,8 +2915,8 @@ func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) { t.Log("Verified pre-deleted files are gone") // Step 4: Now try to delete ALL 5 files using CLI's DeleteFiles API - // - 3 files exist (a3, b1, b2) → should return success - // - 2 files don't exist (a1, a2) → should return failure (404) + // - 3 files exist (f3/a3, f4/b1, f5/b2) → should return success + // - 2 files don't exist (f1/a1, f2/a2) → should return failure (404) var artifactPaths []string for _, f := range testFiles { artifactPaths = append(artifactPaths, tests.RtRepo1+"/delete-partial/"+f) @@ -2921,20 +2929,21 @@ func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) { t.Logf("Delete all 5 files result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err) // Step 5: Verify counts - // 3 files existed (a3, b1, b2) = 3 success - // 2 files didn't exist (a1, a2) = 2 fail (404) + // 3 files existed (f3/a3, f4/b1, f5/b2) = 3 success + // 2 files didn't exist (f1/a1, f2/a2) = 2 fail (404) expectedSuccess := len(testFiles) - len(filesToPreDelete) expectedFail := len(filesToPreDelete) t.Logf("Expected: successCount=%d, failCount=%d", expectedSuccess, expectedFail) t.Logf("Actual: successCount=%d, failCount=%d", totalSuccess, totalFail) assert.Equal(t, expectedSuccess, totalSuccess, - "Should have %d successes (a3, b1, b2 still existed), got %d", expectedSuccess, totalSuccess) + "Should have %d successes (f3/a3, f4/b1, f5/b2 still existed), got %d", expectedSuccess, totalSuccess) assert.Equal(t, expectedFail, totalFail, - "Should have %d failures (a1, a2 already deleted = 404), got %d", expectedFail, totalFail) + "Should have %d failures (f1/a1, f2/a2 already deleted = 404), got %d", expectedFail, totalFail) // Step 6: Verify all files are now gone searchSpec := spec.NewBuilder(). Pattern(tests.RtRepo1 + "/delete-partial/*"). + Recursive(true). BuildSpec() searchCmd := generic.NewSearchCommand() searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) diff --git a/utils/tests/utils.go b/utils/tests/utils.go index b277d706e..329eaf3b9 100644 --- a/utils/tests/utils.go +++ b/utils/tests/utils.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "math/rand" + "net/http" "os" "path/filepath" "regexp" @@ -34,7 +35,6 @@ import ( coreTests "github.com/jfrog/jfrog-cli-core/v2/utils/tests" "github.com/jfrog/jfrog-cli/utils/summary" "github.com/jfrog/jfrog-client-go/artifactory/services" - "github.com/jfrog/jfrog-client-go/artifactory/services/utils" serviceutils "github.com/jfrog/jfrog-client-go/artifactory/services/utils" "github.com/jfrog/jfrog-client-go/auth" "github.com/jfrog/jfrog-client-go/http/jfroghttpclient" @@ -275,13 +275,14 @@ func DeleteFileDirect(serverDetails *config.ServerDetails, artifactoryPath strin } // Check response status - if resp.StatusCode == 204 { + switch resp.StatusCode { + case http.StatusNoContent: // Successfully deleted return true, nil - } else if resp.StatusCode == 404 { + case http.StatusNotFound: // File not found return false, nil - } else { + default: // Other error return false, fmt.Errorf("unexpected status %d: %s", resp.StatusCode, string(body)) } @@ -723,7 +724,7 @@ func CreateSpec(fileName string) (string, error) { return searchFilePath, err } -func ConvertSliceToMap(props []utils.Property) map[string][]string { +func ConvertSliceToMap(props []serviceutils.Property) map[string][]string { propsMap := make(map[string][]string) for _, item := range props { propsMap[item.Key] = append(propsMap[item.Key], item.Value) From 29d810b9852cc1b9a28c8b8d447c7ba79034a87a Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Tue, 13 Jan 2026 05:06:43 +0530 Subject: [PATCH 8/9] Updated fix to use permissions based 403 instead of 404 --- artifactory_test.go | 199 +++++++++++++++++-------------------------- utils/tests/utils.go | 62 ++++++++++++++ 2 files changed, 140 insertions(+), 121 deletions(-) diff --git a/artifactory_test.go b/artifactory_test.go index d8f117530..ed1d98da9 100644 --- a/artifactory_test.go +++ b/artifactory_test.go @@ -2801,146 +2801,89 @@ func TestArtifactoryDeleteByProps(t *testing.T) { cleanArtifactoryTest() } -// TestArtifactoryDeleteCountsNonExistentFiles tests that delete correctly handles -// files that don't exist. The CLI searches first, so non-existent files return -// All 3 files should return 404 on second delete attempt. -// TestArtifactoryDeleteCountsNonExistentFiles tests that delete correctly counts -// 404 failures when all files have already been deleted. -// Files are placed in separate subfolders to prevent path reduction optimization. -func TestArtifactoryDeleteCountsNonExistentFiles(t *testing.T) { - initArtifactoryTest(t, "") - - // Step 1: Upload 3 specific test files in SEPARATE subfolders to prevent path reduction - // Each file goes in its own subfolder so delete service can't optimize to folder delete - testFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in"} - sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in"} - for i, f := range testFiles { - runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-nonexistent/"+f) - } - t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) - - // Step 2: Delete each file using direct DELETE API and verify success (HTTP 204) - for _, f := range testFiles { - artifactPath := tests.RtRepo1 + "/delete-nonexistent/" + f - success, err := tests.DeleteFileDirect(serverDetails, artifactPath) - assert.NoError(t, err, "First delete of %s should not return error", f) - assert.True(t, success, "First delete of %s should succeed (HTTP 204)", f) - } - t.Log("First delete: all files deleted successfully (HTTP 204)") - - // Step 3: Verify all files are now gone via search - searchSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-nonexistent/*"). - Recursive(true). - BuildSpec() - searchCmd := generic.NewSearchCommand() - searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) - reader, err := searchCmd.Search() - assert.NoError(t, err) - remainingFiles, err := reader.Length() - assert.NoError(t, err) - readerCloseAndAssert(t, reader) - assert.Equal(t, 0, remainingFiles, "All files should be deleted") - t.Log("Verified all files are deleted") +// TestArtifactoryDeleteCountsPartialSuccess tests that delete correctly handles +// a mix of successful and failed deletions using CLI's DeleteFiles API. +// Uses permission-based failures (403) to reliably test partial success scenarios. +// - Files in RtRepo1: user has delete permission → success (204) +// - Files in RtRepo2: user has NO delete permission → failure (403) +func TestArtifactoryDeleteCountsPartialSuccess(t *testing.T) { + initArtifactoryTest(t, "") - // Step 4: Try to delete ALL files AGAIN using CLI's DeleteFiles API - // Build list of artifact paths - var artifactPaths []string - for _, f := range testFiles { - artifactPaths = append(artifactPaths, tests.RtRepo1+"/delete-nonexistent/"+f) - } - t.Logf("Attempting second delete of paths: %v", artifactPaths) + // Test user credentials + testUser := "delete-test-user" + testPassword := "DeleteTest123!" - // Use CLI's DeleteFiles API which should properly count 404s as failures - // Error IS expected because all files return 404 - totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(serverDetails, artifactPaths) - // Error is expected when files don't exist (404 responses) - t.Logf("Second delete result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err) + // Step 1: Create a test user + err := tests.CreateUserWithPassword(serverDetails, testUser, testPassword) + assert.NoError(t, err, "Failed to create test user") + t.Logf("Created test user: %s", testUser) - // Step 5: Verify - all 3 files should return 404 (failedCount=3) - expectedSuccess := 0 - expectedFail := len(testFiles) - t.Logf("Expected: successCount=%d, failCount=%d", expectedSuccess, expectedFail) - t.Logf("Actual: successCount=%d, failCount=%d", totalSuccess, totalFail) - assert.Equal(t, expectedSuccess, totalSuccess, - "Second delete should have 0 successes (all files already deleted), got %d", totalSuccess) - assert.Equal(t, expectedFail, totalFail, - "Second delete should have %d failures (all 404), got %d", expectedFail, totalFail) + // Cleanup user at the end + defer func() { + _ = tests.DeleteUser(serverDetails, testUser) + t.Logf("Cleaned up test user: %s", testUser) + }() - // Cleanup - cleanArtifactoryTest() -} + // Step 2: Create permission target - user can delete from RtRepo1 but NOT from RtRepo2 + permissionName := "delete-partial-test-perm" + err = tests.CreatePermissionTarget(serverDetails, permissionName, tests.RtRepo1, testUser, []string{"read", "write", "delete"}) + assert.NoError(t, err, "Failed to create permission target for RtRepo1") + t.Logf("Created permission target: %s (allows delete on %s)", permissionName, tests.RtRepo1) -// TestArtifactoryDeleteCountsPartiallyDeleted tests that delete correctly handles -// a mix of existing and non-existing files using CLI's DeleteFiles API. -// - 3 existing files should return success -// - 2 pre-deleted files should return failure (404) -// Files are placed in separate subfolders to prevent path reduction optimization. -func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) { - initArtifactoryTest(t, "") + // Cleanup permission at the end + defer func() { + _ = tests.DeletePermissionTarget(serverDetails, permissionName) + t.Logf("Cleaned up permission target: %s", permissionName) + }() - // Step 1: Upload 5 specific test files in SEPARATE subfolders to prevent path reduction - // Each file goes in its own subfolder so delete service can't optimize to folder delete - testFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in", "f4/b1.in", "f5/b2.in"} - sourceFiles := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in", "testdata/a/b/b1.in", "testdata/a/b/b2.in"} - for i, f := range testFiles { - runRt(t, "upload", sourceFiles[i], tests.RtRepo1+"/delete-partial/"+f) + // Step 3: Upload files as admin to both repos + // 3 files to RtRepo1 (will succeed with delete) + allowedFiles := []string{"f1/a1.in", "f2/a2.in", "f3/a3.in"} + sourceFilesAllowed := []string{"testdata/a/a1.in", "testdata/a/a2.in", "testdata/a/a3.in"} + for i, f := range allowedFiles { + runRt(t, "upload", sourceFilesAllowed[i], tests.RtRepo1+"/delete-partial/"+f) } - t.Logf("Uploaded %d test files: %v", len(testFiles), testFiles) + t.Logf("Uploaded %d files to %s (delete allowed)", len(allowedFiles), tests.RtRepo1) - // Step 2: Pre-delete 2 files (f1/a1.in and f2/a2.in) using direct DELETE API - filesToPreDelete := []string{"f1/a1.in", "f2/a2.in"} - for _, f := range filesToPreDelete { - artifactPath := tests.RtRepo1 + "/delete-partial/" + f - success, err := tests.DeleteFileDirect(serverDetails, artifactPath) - assert.NoError(t, err, "Pre-delete of %s should not return error", f) - assert.True(t, success, "Pre-delete of %s should succeed (HTTP 204)", f) - t.Logf("Pre-deleted %s successfully (HTTP 204)", f) + // 2 files to RtRepo2 (will fail with 403) + deniedFiles := []string{"f4/b1.in", "f5/b2.in"} + sourceFilesDenied := []string{"testdata/a/b/b1.in", "testdata/a/b/b2.in"} + for i, f := range deniedFiles { + runRt(t, "upload", sourceFilesDenied[i], tests.RtRepo2+"/delete-partial/"+f) } + t.Logf("Uploaded %d files to %s (delete denied)", len(deniedFiles), tests.RtRepo2) - // Step 3: Verify pre-deleted files are gone via search - for _, f := range filesToPreDelete { - searchSpec := spec.NewBuilder(). - Pattern(tests.RtRepo1 + "/delete-partial/" + f). - BuildSpec() - searchCmd := generic.NewSearchCommand() - searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) - reader, err := searchCmd.Search() - assert.NoError(t, err) - count, err := reader.Length() - assert.NoError(t, err) - readerCloseAndAssert(t, reader) - assert.Equal(t, 0, count, "File %s should be deleted", f) - } - t.Log("Verified pre-deleted files are gone") + // Step 4: Create server details for the test user + testUserServerDetails := tests.CreateServerDetailsWithCredentials(serverDetails, testUser, testPassword) - // Step 4: Now try to delete ALL 5 files using CLI's DeleteFiles API - // - 3 files exist (f3/a3, f4/b1, f5/b2) → should return success - // - 2 files don't exist (f1/a1, f2/a2) → should return failure (404) + // Step 5: Build list of all artifact paths (from both repos) var artifactPaths []string - for _, f := range testFiles { + for _, f := range allowedFiles { artifactPaths = append(artifactPaths, tests.RtRepo1+"/delete-partial/"+f) } - t.Logf("Attempting to delete paths: %v", artifactPaths) + for _, f := range deniedFiles { + artifactPaths = append(artifactPaths, tests.RtRepo2+"/delete-partial/"+f) + } + t.Logf("Attempting to delete %d paths with limited user: %v", len(artifactPaths), artifactPaths) - // Error IS expected because some files return 404 - totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(serverDetails, artifactPaths) - // Error is expected when some files don't exist (404 responses) - t.Logf("Delete all 5 files result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err) + // Step 6: Delete using the test user's credentials + // - 3 files from RtRepo1 → should succeed (204) + // - 2 files from RtRepo2 → should fail (403) + totalSuccess, totalFail, err := tests.DeleteFilesByPathsUsingCli(testUserServerDetails, artifactPaths) + // Error IS expected because some files return 403 + t.Logf("Delete result: success=%d, fail=%d, err=%v", totalSuccess, totalFail, err) - // Step 5: Verify counts - // 3 files existed (f3/a3, f4/b1, f5/b2) = 3 success - // 2 files didn't exist (f1/a1, f2/a2) = 2 fail (404) - expectedSuccess := len(testFiles) - len(filesToPreDelete) - expectedFail := len(filesToPreDelete) + // Step 7: Verify counts + expectedSuccess := len(allowedFiles) + expectedFail := len(deniedFiles) t.Logf("Expected: successCount=%d, failCount=%d", expectedSuccess, expectedFail) t.Logf("Actual: successCount=%d, failCount=%d", totalSuccess, totalFail) assert.Equal(t, expectedSuccess, totalSuccess, - "Should have %d successes (f3/a3, f4/b1, f5/b2 still existed), got %d", expectedSuccess, totalSuccess) + "Should have %d successes (files from %s), got %d", expectedSuccess, tests.RtRepo1, totalSuccess) assert.Equal(t, expectedFail, totalFail, - "Should have %d failures (f1/a1, f2/a2 already deleted = 404), got %d", expectedFail, totalFail) + "Should have %d failures (files from %s - 403), got %d", expectedFail, tests.RtRepo2, totalFail) - // Step 6: Verify all files are now gone + // Step 8: Verify files from RtRepo1 are deleted searchSpec := spec.NewBuilder(). Pattern(tests.RtRepo1 + "/delete-partial/*"). Recursive(true). @@ -2949,10 +2892,24 @@ func TestArtifactoryDeleteCountsPartiallyDeleted(t *testing.T) { searchCmd.SetServerDetails(serverDetails).SetSpec(searchSpec) reader, err := searchCmd.Search() assert.NoError(t, err) - finalCount, err := reader.Length() + repo1Count, err := reader.Length() assert.NoError(t, err) readerCloseAndAssert(t, reader) - assert.Equal(t, 0, finalCount, "All files should be deleted") + assert.Equal(t, 0, repo1Count, "All files from %s should be deleted", tests.RtRepo1) + + // Step 9: Verify files from RtRepo2 still exist (delete failed) + searchSpec2 := spec.NewBuilder(). + Pattern(tests.RtRepo2 + "/delete-partial/*"). + Recursive(true). + BuildSpec() + searchCmd2 := generic.NewSearchCommand() + searchCmd2.SetServerDetails(serverDetails).SetSpec(searchSpec2) + reader2, err := searchCmd2.Search() + assert.NoError(t, err) + repo2Count, err := reader2.Length() + assert.NoError(t, err) + readerCloseAndAssert(t, reader2) + assert.Equal(t, len(deniedFiles), repo2Count, "Files from %s should still exist (delete was denied)", tests.RtRepo2) // Cleanup cleanArtifactoryTest() diff --git a/utils/tests/utils.go b/utils/tests/utils.go index 329eaf3b9..dea01a798 100644 --- a/utils/tests/utils.go +++ b/utils/tests/utils.go @@ -347,6 +347,68 @@ func DeleteFilesByPathsUsingCli(serverDetails *config.ServerDetails, artifactory return deleteCmd.DeleteFiles(reader) } +// CreateUserWithPassword creates a new user with the specified password. +func CreateUserWithPassword(serverDetails *config.ServerDetails, username, password string) error { + servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false) + if err != nil { + return err + } + adminFalse := false + userParams := services.NewUserParams() + userParams.UserDetails.Name = username + userParams.UserDetails.Email = username + "@test.com" + userParams.UserDetails.Password = password + userParams.UserDetails.Admin = &adminFalse + return servicesManager.CreateUser(userParams) +} + +// DeleteUser deletes a user. +func DeleteUser(serverDetails *config.ServerDetails, username string) error { + servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false) + if err != nil { + return err + } + return servicesManager.DeleteUser(username) +} + +// CreatePermissionTarget creates a permission target giving the specified user permissions on the specified repo. +func CreatePermissionTarget(serverDetails *config.ServerDetails, permName, repoKey, username string, actions []string) error { + servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false) + if err != nil { + return err + } + params := services.NewPermissionTargetParams() + params.Name = permName + params.Repo = &services.PermissionTargetSection{ + Repositories: []string{repoKey}, + Actions: &services.Actions{ + Users: map[string][]string{ + username: actions, + }, + }, + } + return servicesManager.CreatePermissionTarget(params) +} + +// DeletePermissionTarget deletes a permission target. +func DeletePermissionTarget(serverDetails *config.ServerDetails, permName string) error { + servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false) + if err != nil { + return err + } + return servicesManager.DeletePermissionTarget(permName) +} + +// CreateServerDetailsWithCredentials creates a copy of server details with different credentials. +func CreateServerDetailsWithCredentials(original *config.ServerDetails, username, password string) *config.ServerDetails { + newDetails := *original + newDetails.SetUser(username) + newDetails.SetPassword(password) + // Clear access token if using username/password + newDetails.SetAccessToken("") + return &newDetails +} + // This function makes no assertion, caller is responsible to assert as needed. func GetBuildInfo(serverDetails *config.ServerDetails, buildName, buildNumber string) (pbi *buildinfo.PublishedBuildInfo, found bool, err error) { servicesManager, err := artUtils.CreateServiceManager(serverDetails, -1, 0, false) From 4398c1e2c7e79cdd5e2e0c82b2e7735e04bf4bf7 Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Tue, 13 Jan 2026 05:40:42 +0530 Subject: [PATCH 9/9] Updated dependency --- go.mod | 3 +-- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 737e4c7f0..bdb966365 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,7 @@ require ( github.com/jfrog/build-info-go v1.13.1-0.20260107080257-82671efa69a2 github.com/jfrog/gofrog v1.7.6 github.com/jfrog/jfrog-cli-application v1.0.2-0.20251231144110-a68c3ac11c7a - github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260107090044-56a45e5c560e + github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260113000842-12090b43088f github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260106204841-744f3f71817b github.com/jfrog/jfrog-cli-evidence v0.8.3-0.20251225153025-9d8ac181d615 github.com/jfrog/jfrog-cli-platform-services v1.10.1-0.20251205121610-171eb9b0000e @@ -285,7 +285,6 @@ replace github.com/gfleury/go-bitbucket-v1 => github.com/gfleury/go-bitbucket-v1 //replace github.com/jfrog/jfrog-cli-core/v2 => ../jfrog-cli-core // replace github.com/jfrog/jfrog-cli-artifactory => github.com/fluxxBot/jfrog-cli-artifactory v0.0.0-20260105073552-ae4f86048a11 -replace github.com/jfrog/jfrog-cli-artifactory => github.com/bhanurp/jfrog-cli-artifactory v0.1.12-0.20260108075108-b5389ca4d380 //replace github.com/jfrog/build-info-go => github.com/fluxxBot/build-info-go v1.10.10-0.20260105070825-d3f36f619ba5 // diff --git a/go.sum b/go.sum index 1432bdd10..70c91df11 100644 --- a/go.sum +++ b/go.sum @@ -729,8 +729,6 @@ github.com/beevik/etree v1.6.0 h1:u8Kwy8pp9D9XeITj2Z0XtA5qqZEmtJtuXZRQi+j03eE= github.com/beevik/etree v1.6.0/go.mod h1:bh4zJxiIr62SOf9pRzN7UUYaEDa9HEKafK25+sLc0Gc= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/bhanurp/jfrog-cli-artifactory v0.1.12-0.20260108075108-b5389ca4d380 h1:cj4idl/pC+WMSqj5jddYgrdFfaoWudzpjn0/Zt7ba8U= -github.com/bhanurp/jfrog-cli-artifactory v0.1.12-0.20260108075108-b5389ca4d380/go.mod h1:U/1q7jEO0YGSAWZEZiEmo0lZHI48xBorsFuL/F8C1fU= github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ= github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk= github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= @@ -1218,6 +1216,8 @@ github.com/jfrog/jfrog-apps-config v1.0.1 h1:mtv6k7g8A8BVhlHGlSveapqf4mJfonwvXYL github.com/jfrog/jfrog-apps-config v1.0.1/go.mod h1:8AIIr1oY9JuH5dylz2S6f8Ym2MaadPLR6noCBO4C22w= github.com/jfrog/jfrog-cli-application v1.0.2-0.20251231144110-a68c3ac11c7a h1:XoJ3w2AFi7zniimALNK3idw9bzY9MwB/FM45TMgxYAY= github.com/jfrog/jfrog-cli-application v1.0.2-0.20251231144110-a68c3ac11c7a/go.mod h1:xum2HquWO5uExa/A7MQs3TgJJVEeoqTR+6Z4mfBr1Xw= +github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260113000842-12090b43088f h1:pTUm8bp2vjjKCZI8hqJgIrwEc6veb9FU4hSd7ly7zBs= +github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260113000842-12090b43088f/go.mod h1:U/1q7jEO0YGSAWZEZiEmo0lZHI48xBorsFuL/F8C1fU= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260106204841-744f3f71817b h1:gGGmYXuYvcNns1BnLQI13lC+pgMxrmenx+ramtolQuA= github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260106204841-744f3f71817b/go.mod h1:+Hnaikp/xCSPD/q7txxRy4Zc0wzjW/usrCSf+6uONSQ= github.com/jfrog/jfrog-cli-evidence v0.8.3-0.20251225153025-9d8ac181d615 h1:y5an0bojHL00ipHP1QuBUrVcP+XK+yZHHOJ/r1I0RUM=