Skip to content

Fix file:copy to return error when target file exists without REPLACE_EXISTING #577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DharshiBalasubramaniyam wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix file:copy to return error when target file exists without REPLACE_EXISTING #577

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57b3fcb
Handle file already exists error when replace false
DharshiBalasubramaniyam Oct 7, 2025
5ee5178
[Automated] Update the native jar versions
DharshiBalasubramaniyam Oct 7, 2025
3c8b115
Fix FileNotFoundError when removing source file
DharshiBalasubramaniyam Oct 7, 2025
a8b03de
Refactor testCopyFileToNonExistentFileReplaceFalse test cases to use …
DharshiBalasubramaniyam Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ballerina/Dependencies.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ dependencies = [
[[package]]
org = "ballerina"
name = "os"
version = "1.10.0"
version = "1.10.1"
dependencies = [
{org = "ballerina", name = "io"},
{org = "ballerina", name = "jballerina.java"}
Expand Down
49 changes: 34 additions & 15 deletions ballerina/tests/file-test.bal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,13 @@ function testCreateDirWithoutParentDir() {
}

@test:Config {}
function testCopyFile() {
function testCopyFileToNonExistentFileReplaceFalse() {
error? removeResult = remove(tmpdir + copyFile);
if removeResult is error && removeResult !is FileNotFoundError {
io:println(">>>> " + removeResult.toString());
test:assertFail("Error removing test resource!");
}

Copy link
Member

@ThisaruGuruge ThisaruGuruge Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this logic? Can't we just do this?

Suggested change
function testCopyFileToNonExistentFileReplaceFalse() {
error? removeResult = remove(tmpdir + copyFile);
if removeResult is error && removeResult !is FileNotFoundError {
io:println(">>>> " + removeResult.toString());
test:assertFail("Error removing test resource!");
}
function testCopyFileToNonExistentFileReplaceFalse() returns error? {
check remove(tmpdir + copyFile);

Copy link
Contributor Author

@DharshiBalasubramaniyam DharshiBalasubramaniyam Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before copying the file without REPLACE_EXISTING, we need to ensure that the file not already exist, Thats why i am removing the file early is it already exists.

remove() can fail for two different reasons:

  • FileNotFoundError – which is expected (the file already doesn’t exist → fine).
  • Other errors – e.g., permission denied, file locked, or I/O issues → real problem.

When we write check remove(...), any error (including FileNotFoundError) will cause the test to fail immediately — even though that particular error is not actually a problem for this setup.

The current code distinguishes between those cases:

  • If removal fails because the file doesn’t exist, that’s fine — proceed.
  • If removal fails for any other reason, the test should fail — because that could interfere with the copy test.

We’re making sure the file is gone before copying — if it’s not there, fine; if we can’t remove it for another reason, fail.”

Copy link
Member

@ThisaruGuruge ThisaruGuruge Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is not ideal in that case, because it tests multiple things.

Why do we need to remove the file first?
Maybe what we should do is to remove the file using a BeforeTest function. Shall we do that?

Copy link
Contributor Author

@DharshiBalasubramaniyam DharshiBalasubramaniyam Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now changed the test case to use a BeforeTest function

I removing the file first because, the test is testCopyFileToNonExistentFileReplaceFalse(); Copy a file to a non-existent destination file, without using REPLACE_EXISTING

So, before running the test, we need to guarantee that the destination (tmpdir + copyFile) truly does not exist — otherwise the test wouldn’t actually be testing the correct scenario.

MetaData|error srcmetadata = getMetaData(srcFile);
if srcmetadata is MetaData {
srcFileLength = srcmetadata.size;
Expand All @@ -267,41 +273,54 @@ function testCopyFile() {
}
}

@test:Config {dependsOn: [testCopyFile]}
function testCopyFileReplaceFalse() {
MetaData|error srcMmetadata = getMetaData(srcModifiedFile);
if srcMmetadata is MetaData {
srcModifiedFileLength = srcMmetadata.size;
@test:Config {dependsOn: [testCopyFileToNonExistentFileReplaceFalse]}
function testCopyFileToExistingFileReplaceFalse() {
error? copyResult = copy(srcFile, tmpdir + copyFile);
if copyResult is error {
string expectedErrMsg = "The target file already exists";
test:assertTrue(copyResult.message().includes(expectedErrMsg));
} else {
test:assertFail("Error retrieving source file size!");
test:assertFail("File copy succeeded to an existing file when replace false!");
}
}

error? copyResult = copy(srcModifiedFile, tmpdir + copyFile);
@test:Config {dependsOn: [testCopyFileToExistingFileReplaceFalse]}
function testCopyFileToExistingFileReplaceTrue() {
MetaData|error srcMetadata = getMetaData(srcModifiedFile);
if srcMetadata is MetaData {
srcModifiedFileLength = srcMetadata.size;
} else {
test:assertFail("Error retrieving source file size!");
}
error? copyResult = copy(srcModifiedFile, tmpdir + copyFile, REPLACE_EXISTING);
if copyResult is error {
test:assertFail("File not copied!");
} else {
MetaData|error metadata = getMetaData(tmpdir + copyFile);
if metadata is MetaData {
int destFileLength = metadata.size;
test:assertEquals(destFileLength, srcFileLength, "File size mismatch!");
test:assertNotEquals(destFileLength, srcModifiedFileLength);
test:assertEquals(destFileLength, srcModifiedFileLength, "File size mismatch!");
test:assertNotEquals(destFileLength, srcFileLength);
error? removeResult = remove(tmpdir + copyFile);
if removeResult is error {
test:assertFail("Error removing test resource!");
}
} else {
test:assertFail("Error retrieving destination file size!");
}
}
}

@test:Config {dependsOn: [testCopyFileReplaceFalse]}
function testCopyFileReplaceTrue() {
error? copyResult = copy(srcModifiedFile, tmpdir + copyFile, REPLACE_EXISTING);
@test:Config {dependsOn: [testCopyFileToExistingFileReplaceTrue]}
function testCopyFileToNonExistentFileReplaceTrue() {
error? copyResult = copy(srcFile, tmpdir + copyFile, REPLACE_EXISTING);
if copyResult is error {
test:assertFail("File not copied!");
} else {
MetaData|error metadata = getMetaData(tmpdir + copyFile);
if metadata is MetaData {
int destFileLength = metadata.size;
test:assertEquals(destFileLength, srcModifiedFileLength, "File size mismatch!");
test:assertNotEquals(destFileLength, srcFileLength);
test:assertEquals(destFileLength, srcFileLength, "File size mismatch!");
error? removeResult = remove(tmpdir + copyFile);
if removeResult is error {
test:assertFail("Error removing test resource!");
Expand Down
5 changes: 4 additions & 1 deletion native/src/main/java/io/ballerina/stdlib/file/nativeimpl/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,9 @@ public static Object copy(BString sourcePath, BString destinationPath, BString..
} catch (NoSuchFileException ex) {
return FileUtils.getBallerinaError(FileConstants.FILE_NOT_FOUND_ERROR,
"The target directory does not exist: " + ex.getMessage());
} catch (FileAlreadyExistsException ex) {
return FileUtils.getBallerinaError(FileConstants.INVALID_OPERATION_ERROR,
"The target file already exists: " + ex.getMessage());
} catch (IOException ex) {
return FileUtils.getBallerinaError(FileConstants.FILE_SYSTEM_ERROR,
"An error occurred when copying the file/s: " + ex.getMessage());
Expand Down Expand Up @@ -335,7 +338,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
Path newFile = target.resolve(source.relativize(file));
try {
Files.copy(file, newFile, copyOptions);
} catch (NoSuchFileException e) {
} catch (IOException e) {
throw e;
} catch (Exception e) {
log.debug(e.getMessage());
Expand Down