fix: delete folder issue #393

[jayennis22]

It's not clear to me how the original JMESPath search expression ever worked, but it clearly fails to extract any Keys from the JSON returned by listObjects. This new search expression works in every case I have found to test with.

Fixes #393

[CLAassistant]

All committers have signed the CLA.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[dj4oC]

• Security: no findings — no SQL/path-traversal/CSRF surface; single JMESPath expression change in batchDelete().
• Stability: the fix looks correct. Contents/*/Key is not valid JMESPath for extracting a Key field from each element of the Contents list returned by S3's listObjects; Contents[].Key is the standard flatten-projection syntax and is what actually returns a flat array of keys. As described, the old expression silently returned nothing, so batchDelete() hit the empty($keys) → continue branch and never issued the delete — a real, plausible bug (issue #393). No \OC\ private API usage, no new dependency.
• Performance: no findings; behavior is a bug fix, not a hot-path change.
• Test coverage: no unit test added for this JMESPath extraction. Given this fixes a bulk-delete correctness bug, a regression test mocking listObjects's response shape and asserting batchDelete() extracts the right keys would be valuable to prevent silent regressions here again.
• TODOs found: 0
• Dependency touched: no
• CI status: no checks recorded for this PR (fork PR from 2023, predates current Actions CI — would need a rebase to pick up current CI)

Verdict

The fix itself is correct and low-risk (single-line, matches documented JMESPath syntax), but the PR is stale (2023) and untested by current CI. Recommend the author rebase onto current master to pick up CI, and ideally add a regression test for batchDelete() before merge. Not blocking on the fix's correctness, but not mergeable without a rebase.

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code

[phil-davis]

https://github.com/owncloud/files_external_s3/actions/runs/29554941529/job/87808821313?pr=1048

1) OCA\Files_External\Tests\Storage\Amazons3Test::testDirectories with data set #0 ('folder')
The folder 'folder' could not be deleted
Failed asserting that false is true.

/home/runner/work/files_external_s3/files_external_s3/tests/lib/Files/Storage/Storage.php:128

A simple delete of a folder is not successful.

Note: test results after that are not so relevant, because folders cannot be deleted, and their existence pollutes the test environment of later test scenarios.