fix(service/fs): handle if_not_exists flag to raise ConditionNotMatch error

[kingsword09]

Which issue does this PR close?

Closes #.

Rationale for this change

While implementing the WriteOptions#6322 in Node.js, I found that the if_not_exists property test fails without throwing the ConditionNotMatch exception.

What changes are included in this PR?

When if_not_exists is true, if the file already exists, a ConditionNotMatch exception should be thrown.

opendal/core/tests/behavior/async_write.rs

Lines 773 to 778 in 23f01dd

	let res = op
	.write_with(&path, content.clone())
	.if_not_exists(true)
	.await;
	assert!(res.is_err());
	assert_eq!(res.unwrap_err().kind(), ErrorKind::ConditionNotMatch);

Are there any user-facing changes?

[Xuanwo]

This should have been catched at

opendal/core/src/services/fs/core.rs

Lines 146 to 152 in 577912d

	let mut open_options = tokio::fs::OpenOptions::new();
	if op.if_not_exists() {
	open_options.create_new(true);
	} else {
	open_options.create(true);
	}

Maybe there are other issues?

[kingsword09]

@Xuanwo At first, I also thought this part was functional, but there's an extra tmp_path here. When the atomic_write_dir exists, a tmp_path gets generated, causing every write operation to use new tmp_path instead. As a result, the ConditionNotMatch exception won't be thrown.

opendal/core/src/services/fs/core.rs

Line 162 in 577912d

.open(tmp_path.unwrap_or(target_path))

[Xuanwo]

@Xuanwo At first, I also thought this part was functional, but there's an extra tmp_path here. When the atomic_write_dir exists, a tmp_path gets generated, causing every write operation to use new tmp_path instead. As a result, the ConditionNotMatch exception won't be thrown.

opendal/core/src/services/fs/core.rs

Line 162 in 577912d

.open(tmp_path.unwrap_or(target_path))

I see. So the real fix should use the correct open options to open the target path. In this way, we can avoid an extra syscall.

[kingsword09]

I see. So the real fix should use the correct open options to open the target path. In this way, we can avoid an extra syscall.

Change to check if if_not_exists is true before creating tmp_path. If true, do not create the tmp_path temporary file.

[Xuanwo]

Change to check if if_not_exists is true before creating tmp_path. If true, do not create the tmp_path temporary file.

Wow, you're so clever! Could you add some comments to explain it a bit? I spent some time to understand how it works.

[kingsword09]

In the code, the ‎if_not_exists flag is used to ensure that the write operation only happens if the target file does not already exist. If ‎if_not_exists = true, we want to throw a ‎ConditionNotMatch exception when the target file exists. In the previous implementation, a ‎tmp_path temporary file was always created, so each write operation was actually performed on a new temporary file. As a result, the existence of the target file was never checked, and the exception was not triggered as expected.

The core of this change is to check whether ‎if_not_exists is true before creating the temporary file (‎tmp_path). If it is, we try to write directly to the target path using ‎OpenOptions::create_new(true), which guarantees that the write will only succeed if the file does not already exist. If the target file exists, a ‎ConditionNotMatch error is thrown. This avoids the problem of always writing to a temporary file and missing the existence check.

In short:
• The previous logic always created a temporary file, so it couldn’t detect if the target file existed.
• Now, we check ‎if_not_exists first. If it’s true, we try to write directly to the target path, so the error can be thrown correctly and the expected behavior is achieved.

[Xuanwo]

Hi, this could be logically incorrect since atomic_write_dir is meant to ensure our write operation is atomic. Even if the target path is empty, we still need to write to the temporary path first.

I think that in the case of atomic_write_dir, it's unavoidable to call try_exists first. The logic here is somewhat complex, so perhaps we should consider a major refactor.

[Xuanwo]

cc @kingsword09, after giving it some thought, I realize the current behavior isn't what I expected.

[kingsword09]

You're right, it might really need to be refactored here to make more sense.

[kingsword09]

After merging #6327, simply adjusting the should_append code as shown below can temporarily resolve the issue, removing the need for an additional try_exists check since the append operation inherently includes a verification step.

before:

opendal/core/src/services/fs/core.rs

Lines 127 to 130 in 8f277e9

	let should_append = op.append()
	&& tokio::fs::try_exists(&target_path)
	.await
	.map_err(new_std_io_error)?;

after:

let should_append = (op.append() || op.if_not_exists())
                && tokio::fs::try_exists(&target_path)
                    .await
                    .map_err(new_std_io_error)?;

[Xuanwo]

Hi, @kingsword09 I did the refactor on your PR directly. Let's see how it works.

[Xuanwo]

Thank you @kingsword09 for inspiring me about this work.

[kingsword09]

I tested locally and two tests would fail

[Xuanwo]

I tested locally and two tests would fail

Let me take a look.