796953 - [OS.File] writeAtomic should accept an option noOverwrite

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Just as |copy| and |move| accept an option |noOverwrite|, so should |writeAtomic|.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Should anyone decide to take this as a mentored bug, it requires modifying function |OS.File.writeAtomic|, defined in osfile_shared_front.jsm.

Comment 2

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Local tests are running fine.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 667729 [details] [diff] [review]
Patch v1

Review of attachment 667729 [details] [diff] [review]:
-----------------------------------------------------------------

This looks quite good, thanks. I would like the following changes done, though.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +627,5 @@
>   * - {number} offset The offset in |buffer| at which to start extracting
>   * data
>   * - {string} tmpPath The path at which to write the temporary file.
> + * - {bool} noOverwrite - If set, this function will fail if a file already
> + * exists at |path|.

You should also document whether |tmpPath| is overwritten if |path| exists.
(also in osfile_shared_front.jsm)

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +376,5 @@
> +  } catch (x) {
> +    OS.File.remove(tmpPath);
> +    throw x;
> +  }
> +

Your code fails at the last possible moment. This is semantically sound but I tend to assume that this is not what users want. So, I would add a check before opening |tmpFile| to fail fast if there is already a file at |path|.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +508,5 @@
>          test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed");
>        }
>      );
>  
> +// Check that writeAtomic fails if noOverwrite is true

... and the destination file already exists!

@@ +512,5 @@
> +// Check that writeAtomic fails if noOverwrite is true
> +
> +    promise = promise.then(
> +      function check_with_noOverwrite() {
> +        options = {tmpPath: tmpPath, bytes: bytes -100, noOverwrite: true};

This looks correct. However, we have slightly changed the recommended style while you were working on this bug. Could you replace this |bytes - 100| with some use of method |.subarray()|? See the latest version of tests for examples.

(same thing in worker_test_osfile_front.js)

@@ +513,5 @@
> +
> +    promise = promise.then(
> +      function check_with_noOverwrite() {
> +        options = {tmpPath: tmpPath, bytes: bytes -100, noOverwrite: true};
> +        optionsBackup = {tmpPath: tmpPath, bytes: bytes -100, noOverwrite: true};

I don't think that you actually use |optionsBackup|.

@@ +520,5 @@
> +    );
> +
> +    promise = promise.then(
> +      function onSuccess() {
> +        test.fail("With noOverwrite, writeAtomic should have failed");

"should have refused to overwrite file"

@@ +522,5 @@
> +    promise = promise.then(
> +      function onSuccess() {
> +        test.fail("With noOverwrite, writeAtomic should have failed");
> +      },
> +      function onFailure() {

You could improve the test further by checking the reason of failure and ensure that it is |becauseFileExists|.

@@ +523,5 @@
> +      function onSuccess() {
> +        test.fail("With noOverwrite, writeAtomic should have failed");
> +      },
> +      function onFailure() {
> +        test.info("With noOverwrite, writeAtomic failure");

"With noOverwrite, writeAtomic correctly failed"

@@ +530,5 @@
> +    );
> +
> +    promise = promise.then(
> +      function compare_complete() {
> +        test.info("Compare complete");

"With noOverwrite, writeAtomic correctly did not overwrite destination file"

Comment 4

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Updated patch with suggested changes.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 668519 [details] [diff] [review]
Patch v2

Review of attachment 668519 [details] [diff] [review]:
-----------------------------------------------------------------

Patch is good, but we still need to finalize the matter of checking whether a file already exists.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +341,5 @@
> +  let noOverwrite = options.noOverwrite;
> +  if (noOverwrite) {
> +    try {
> +      // Check if file exists and try to move it to get the error. 
> +      OS.File.read(path);

|OS.File.read| is a little overkill, as it reads the full contents of the file.

Let me suggest the following:
- introduce a function |OS.File.exists|, which uses |OS.File.open| to determine whether a file can be opened;
- use |OS.File.exists| here;
- in a followup bug, we will see about optimizing |OS.File.exists| using e.g. |access()|

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

I added the exists method. However, I still need to use the move method to get the 'becauseExists' error. 
I tried to do something like throw new OS.File.Error('writeAtomic'), but the 'becauseExists' property is platform specific (Constants.libc.EEXIST or Constants.Win.ERROR_FILE_EXISTS).  

> if (noOverwrite && OS.File.exists(path)) {
>   // Try to move the file to get the appropiate file exists error.
>   OS.File.move(tmpPath, path, {noCopy: true, noOverwrite: true});
> }

In the follow up bug I will create the tests for the exists method.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 669268 [details] [diff] [review]
Patch v3

Review of attachment 669268 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +356,5 @@
>    }
> +
> +  let noOverwrite = options.noOverwrite;
> +  if (noOverwrite && OS.File.exists(path)) {
> +    // Try to move the file to get the appropiate file exists error.

Ok, I see why you do this, but this is too much of a hack :)

The right solution is... to complicate things (again) by introducing a function |OS.File.Error.exists()|, along the lines of |OS.File.Error.closed()|. See the end of files osfile_win_allthreads.jsm and osfile_unix_allthreads.jsm.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Andres Hernandez [:andreshm] from comment #6)
> Created attachment 669268 [details] [diff] [review]
> Patch v3
> 
> I added the exists method. However, I still need to use the move method to
> get the 'becauseExists' error. 
> I tried to do something like throw new OS.File.Error('writeAtomic'), but the
> 'becauseExists' property is platform specific (Constants.libc.EEXIST or
> Constants.Win.ERROR_FILE_EXISTS). 

Yes, I encountered this issue for reporting that a file is closed, which lead me to define OS.File.Error.closed. See osfile_{win, unix}_allthreads.jsm . You will need to do the same.

> In the follow up bug I will create the tests for the exists method.

Sounds good.

Comment 9

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Added the suggested change.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 669630 [details] [diff] [review]
Patch v4

Review of attachment 669630 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
You have my r+ (assuming tests pass, of course).

Comment 11

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e77e0c7a59a5

Comment 12

[Andres Hernandez [:andreshm]]

Try finished fine with some oranges https://tbpl.mozilla.org/?tree=Try&rev=e77e0c7a59a5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/268128a99ac9

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/268128a99ac9