Skip to content

Commit

Permalink
Don't allow overwrite of files/directories using move()
Browse files Browse the repository at this point in the history 
Depends on D150663

Differential Revision: https://phabricator.services.mozilla.com/D157643

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1791384
gecko-commit: e0355ac9047673122eb9bc6876a06d4c5ddcd79d
gecko-reviewers: dom-storage-reviewers, jari
  • Loading branch information
@moz-wptsync-bot
Randell Jesup authored and moz-wptsync-bot committed Sep 21, 2022
1 parent 8b48818 commit b20e834
Showing 1 changed file with 13 additions and 17 deletions.
30 changes: 13 additions & 17 deletions fs/script-tests/FileSystemFileHandle-move.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,11 @@ directory_test(async (t, root) => {
await promise_rejects_dom(
t, 'NoModificationAllowedError', handle.move('file-after'));

// Can move handle once the writable is closed.
// Can't move handle once the writable is closed.
await stream.close();
await handle.move('file-after');
assert_array_equals(await getSortedDirectoryEntries(root), ['file-after']);
await promise_rejects_dom(
t, 'NoModificationAllowedError', handle.move('file-after'));

This comment has been minimized.

Sign in to view

Copy link
@a-sully

a-sully Oct 3, 2022

Contributor

Can we make this InvalidModifcationError? NoModificationAllowedError currently maps exclusively to file locking errors (at least within the OPFS) and it would be nice to provide that guarantee to developers.

InvalidModifcationError is more accurate anyways. According to the DOMException:

NoModificationAllowedError: The object cannot be modified
InvalidModifcationError: The object cannot be modified in this way

The issue isn't that the file can't be moved, but that it can't be moved there

This comment has been minimized.

Sign in to view

Copy link
@jesup

jesup Oct 7, 2022

Contributor

Ok. Can you update the move PR?

This comment has been minimized.

Sign in to view

Copy link
@a-sully

a-sully Nov 2, 2022

Contributor

Just updated the PR. It's still somewhat WIP because we presumably need to specify what happens to children on directory moves (and the new locking paradigm) but all the open questions should be addressed in the algorithm section
assert_array_equals(await getSortedDirectoryEntries(root), ['file-before']);

This comment has been minimized.

Sign in to view

Copy link
@a-sully

a-sully Oct 3, 2022

Contributor

I don't understand this assertion. If we're not allowing overwrites, why is 'file-after' gone? Shouldn't this be

assert_array_equals(await getSortedDirectoryEntries(root), ['file-after', 'file-before']);

This comment has been minimized.

Sign in to view

Copy link
@jesup

jesup Oct 7, 2022

Contributor

Corrected

This comment has been minimized.

Sign in to view

Copy link
@a-sully

a-sully Nov 2, 2022

Contributor

It looks like this is still not correct?
}, 'move(name) while the destination file has an open writable fails');


Expand Down Expand Up @@ -308,15 +309,11 @@ directory_test(async (t, root) => {
// Assert the file is still in the source directory.
assert_array_equals(await getSortedDirectoryEntries(dir_src), ['file']);

// Can move handle once the writable is closed.
// Can't move handle once the writable is closed.
await stream.close();
await file.move(dir_dest);
assert_array_equals(
await getSortedDirectoryEntries(root), ['dir-dest/', 'dir-src/']);
assert_array_equals(await getSortedDirectoryEntries(dir_src), []);
assert_array_equals(await getSortedDirectoryEntries(dir_dest), ['file']);

This comment has been minimized.

Sign in to view

Copy link
@a-sully

a-sully Oct 3, 2022

Contributor

Why are we no longer asserting that the destination directory looks as we expect?

This comment has been minimized.

Sign in to view

Copy link
@jesup

jesup Oct 7, 2022

Contributor

See updates; rewrote this test to be explicit about testing overwrites
assert_equals(await getFileContents(file), 'abc');
assert_equals(await getFileSize(file), 3);
await promise_rejects_dom(
t, 'NoModificationAllowedError', file.move(dir_dest));
assert_array_equals(await getSortedDirectoryEntries(dir_src), ['file']);
}, 'move(dir) while the destination file has an open writable fails');

directory_test(async (t, root) => {
Expand All @@ -336,13 +333,12 @@ directory_test(async (t, root) => {
// Assert the file is still in the source directory.
assert_array_equals(await getSortedDirectoryEntries(dir_src), ['file-src']);

// Can move handle once the writable is closed.
// Can't move handle once the writable is closed.
await stream.close();
await file.move(dir_dest, 'file-dest');
assert_array_equals(
await getSortedDirectoryEntries(root), ['dir-dest/', 'dir-src/']);
assert_array_equals(await getSortedDirectoryEntries(dir_src), []);
assert_array_equals(await getSortedDirectoryEntries(dir_dest), ['file-dest']);
await promise_rejects_dom(
t, 'NoModificationAllowedError', file.move(dir_dest, 'file-dest'));
// Assert the file is still in the source directory.
assert_array_equals(await getSortedDirectoryEntries(dir_src), ['file-src']);
assert_equals(await getFileContents(file), 'abc');
assert_equals(await getFileSize(file), 3);
}, 'move(dir, name) while the destination file has an open writable fails');

3 comments on commit b20e834

@a-sully
Copy link
Contributor

@a-sully a-sully commented on b20e834 Oct 3, 2022

Choose a reason for hiding this comment

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

@jesup could you please test the "don't allow overwrite on move" behavior in separate tests? It's unclear from the test names (moving with an open writable) that the issue is not allowing overwrites

@jesup
Copy link
Contributor

@jesup jesup commented on b20e834 Oct 7, 2022

Choose a reason for hiding this comment

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

My next update should handle that; and remove the destination-has-a-writable tests since they're irrelevant if you don't allow overwrites

@a-sully
Copy link
Contributor

@a-sully a-sully commented on b20e834 Nov 2, 2022

Choose a reason for hiding this comment

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

My next update should handle that; and remove the destination-has-a-writable tests since they're irrelevant if you don't allow overwrites

Any updates on this?

Please sign in to comment.