dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.11k stars 1.56k forks source link

[library/io] File behavior on Links #53905

Open sgrekhov opened 11 months ago

sgrekhov commented 11 months ago

According to the File.create/createSync() documentation

If exclusive is true and to-be-created file already exists, this operation completes the future with a PathExistsException.

If exclusive is false, existing files are left untouched by create. Calling create on an existing file still might fail if there are restrictive permissions on the file.

But what should we expect if we create a file and there is an existing link with the same path? The current behavior is as if we are working with link's target. That means that if exclusive is true and there is an existing link to a directory/file/not existing entity then operation fails.

If exclusive is false and there is an existing link pointing to a directory, then operation fails. If there is an existing link to a file, then File with the same path is created and all operations like read/write are performed on link's target. If there is an existing link to a not existing file, then File.create() creates link's target.

Tested on MacOs.

Is all of the above intended?

cc @brianquinlan, @lrhn.

brianquinlan commented 11 months ago

That's what I would expect to happen because I'm imagining that this is all interpreted in terms of the open system call ;-)

Probably the documentation change that we need is:

- If exclusive is true and to-be-created file already exists, this operation
- completes the future with a PathExistsException.
+ If exclusive is true and there is already a file system entity at the to-be-created
+ path, then createSync throws a PathExistsException.
```diff
- If exclusive is false, existing files are left untouched by createSync.
- Calling createSync on an existing file still might fail if there are restrictive
- permissions on the file.
+ If exclusive is false, existing file system entities are left untouched by
+ createSync. If the to-be-created [path] is a directory, then createSync will
+ throw a FileSystemException. If the to-be-created [path] is a Link, then 
+ createSync will throw unless the Link can be resolved into a File.
+ Calling createSync on an existing file still might fail if there are restrictive 
+ permissions on the file.

At the same time, this should be updated:

- If exclusive is true and to-be-created file already exists, this operation 
- completes the future with a FileSystemException.
+ If exclusive is true and to-be-created file already exists, this operation throws a + FileSystemException.
sgrekhov commented 11 months ago

...the Link can be resolved into a File.

Does it assumes that if Link target does not exists then it is created by create()? If not maybe it makes sense to add it?

sgrekhov commented 11 months ago

And one more that surprised me and what, I think, it makes sense to reflect in the documentation. If we create File from Link, then read/write to that File will read/write link's target. But if we delete the File, then the target of the link is not deleted. Below the code (copy from my test, but I hope it's understandable)

  File target = getTempFileSync(parent: sandbox);
  target.writeAsStringSync("Target content");
  Link link = getTempLinkSync(parent: sandbox, target: target.path);
  File file = File(link.path);
  await file.create(recursive: recursive).then((File created) {
    Expect.isTrue(created.existsSync());
    Expect.equals(file.path, created.path);
    // Now check that all read/write operations are performed on link's target
    Expect.equals("Target content", file.readAsStringSync());
    created.writeAsStringSync("Lily was here");
    Expect.equals("Lily was here", target.readAsStringSync());
    created.deleteSync();
    Expect.isFalse(file.existsSync());
    Expect.isTrue(target.existsSync());
    asyncEnd();
  });
brianquinlan commented 11 months ago

I think that created.deleteSync() is probably deleting the symlink.

sgrekhov commented 11 months ago

No, it doesn't. I can't test it on Windows and Linux now, but on MacOs it doesn't. Please see example above, it works. The full test can be found here https://github.com/sgrekhov/co19/blob/co19-2303-1/LibTest/io/File/create_A03_t03.dart

brianquinlan commented 11 months ago

No, it doesn't. I can't test it on Windows and Linux now, but on MacOs it doesn't. Please see example above, it works. The full test can be found here https://github.com/sgrekhov/co19/blob/co19-2303-1/LibTest/io/File/create_A03_t03.dart

In that example, where are you checking that the link exists?

sgrekhov commented 11 months ago

Ah, sorry. I misunderstand you.

print(link.existsSync()); // false

So yes, created.deleteSync() deletes the symlink. I was wrong

sgrekhov commented 11 months ago

So, that means that

File file = File(someLink.path);
file.create(exclusive: false);

creates someLink.target if it doesn't exists. But

file.delete();

deletes the symlink, not the target. If this is as designed, I think, it should be clearly specified in the documentation

brianquinlan commented 11 months ago

I filed an issue to track the delete behavior: https://github.com/dart-lang/sdk/issues/53917

brianquinlan commented 11 months ago

...the Link can be resolved into a File.

Does it assumes that if Link target does not exists then it is created by create()? If not maybe it makes sense to add it?

I think that, at some point, we are documenting and testing edge-case behavior that isn't going to be important for the vast majority of our users. The behavior of create when path is a broken symlink seems like one such case.