Skip to content

✨ Add recursive delete for non-empty directories - #641

Open
anggrayudi wants to merge 1 commit into
vinceglb:mainfrom
anggrayudi:fix/640-delete-non-empty-folders
Open

✨ Add recursive delete for non-empty directories#641
anggrayudi wants to merge 1 commit into
vinceglb:mainfrom
anggrayudi:fix/640-delete-non-empty-folders

Conversation

@anggrayudi

Copy link
Copy Markdown

Closes #640.

delete() currently fails on a directory that still has contents, so callers end up writing their own recursive extension — which is exactly what the issue reports.

What changed

delete() gains a recursively flag:

public expect suspend fun PlatformFile.delete(
    mustExist: Boolean = true,
    recursively: Boolean = false,
)

It defaults to false, so every existing call behaves exactly as before and a non-empty directory still fails. Nothing starts deleting trees by surprise.

Symlinks

The issue asks for links to be unlinked rather than followed, and this turned out to be the part that actually needs care. kotlinx-io's FileMetadata carries no link information, and isDirectory() resolves the link and reports on the target — so a naive recursion walks into a symlink and deletes the contents of whatever it points at, outside the tree the caller asked to remove.

So there is a new internal isSymbolicLink(), answered with the primitive that does not resolve the link:

Target How
JVM Files.isSymbolicLink
Android Os.lstat + OsConstants.S_ISLNKjava.nio.file.Files needs API 26 and this library supports 21
Apple attributesOfItemAtPath (does not resolve the link)
Linux lstat
Windows GetFileAttributesW + FILE_ATTRIBUTE_REPARSE_POINT

The SAF branch on Android is deliberately untouched: there is no empty-directory rule to work around there, and removing a document is the provider's job.

Tests

Four cases in nonWebTest, so they run on JVM, iOS simulator, macOS and Android host: a non-empty directory still fails without the flag, a nested tree is removed with it, a plain file is fine either way, and a missing path with mustExist = false stays a no-op.

The symlink case is in jvmTest, since creating a link needs a platform API: it builds a directory containing a symlink to a folder outside it, deletes recursively, and asserts the outside folder's contents survive.

I checked that test is load-bearing rather than trusting it — removing the isSymbolicLink() guard makes it fail on exactly the right assertion ("the symlink target lives outside that tree and is untouched"), and it passes again once restored.

Verification

  • ./gradlew assemble — all four published modules assemble.
  • Tests green on jvmTest, iosSimulatorArm64Test, macosArm64Test and testAndroidHostTest (209 tests, 0 failures).
  • linuxX64 and mingwX64 compile, cross-compiled from macOS. I could not run tests on those two, so their isSymbolicLink() is compile-verified and reasoned about, not executed — worth a look from someone with those machines.
  • I could not run the ktlint command from AGENTS.md: ktlint is not on my PATH and ktlint-compose-0.4.28-all.jar is not in the repo, so style is matched by hand. Happy to fix anything CI flags.

Two notes on the OOM and JS failures I hit while running assemble: both were local environment problems (a full disk, then an 8 GB Kotlin daemon heap on the sample's release framework link), not related to this change.

🤖 Generated with Claude Code

delete() gains a `recursively` flag, defaulting to false, so existing
calls behave exactly as before and a non-empty directory still fails.

Symlinks are unlinked rather than followed. kotlinx-io's FileMetadata
carries no link information and isDirectory() resolves the link, so a
naive recursion would delete the contents of whatever the link points
at. A new internal isSymbolicLink() answers that per platform:
Files.isSymbolicLink on JVM, Os.lstat on Android (java.nio needs API 26
and this library supports 21), attributesOfItemAtPath on Apple, lstat on
Linux, and the reparse point attribute on Windows.

The SAF branch on Android is left alone: removing a document is the
provider's job and there is no empty-directory rule to work around.

Closes vinceglb#640

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle deletion of non-empty folders.

1 participant