Skip to content
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.

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

fs: fix fs.openAsBlob, add fs.openAsBlobSync and fsPromises.openAsBlob #49759

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
91 changes: 65 additions & 26 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,44 @@ by [Naming Files, Paths, and Namespaces][]. Under NTFS, if the filename contains
a colon, Node.js will open a file system stream, as described by
[this MSDN page][MSDN-Using-Streams].

### `fsPromises.openAsBlob(path[, options])`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

* `path` {string|Buffer|URL}
* `options` {Object}
* `type` {string} An optional mime type for the blob.
* Return: {Promise} containing {Blob}

Returns a {Blob} whose data is backed by the given file.

The file must not be modified after the {Blob} is created. Any modifications
will cause reading the {Blob} data to fail with a `DOMException` error.
Synchronous stat operations on the file when the `Blob` is created, and before
each read in order to detect whether the file data has been modified on disk.

```mjs
import { openAsBlob } from 'node:fs/promises';

const blob = await openAsBlob('the.file.txt');
const ab = await blob.arrayBuffer();
blob.stream();
```

```cjs
const { openAsBlob } = require('node:fs/promises');

(async () => {
const blob = await openAsBlob('the.file.txt');
const ab = await blob.arrayBuffer();
blob.stream();
})();
```

### `fsPromises.opendir(path[, options])`

<!-- YAML
Expand Down Expand Up @@ -3401,43 +3439,27 @@ a colon, Node.js will open a file system stream, as described by
Functions based on `fs.open()` exhibit this behavior as well:
`fs.writeFile()`, `fs.readFile()`, etc.

### `fs.openAsBlob(path[, options])`
### `fs.openAsBlob(path[, options], callback)`

<!-- YAML
added: v19.8.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49759
description: Replaced with callback-based function.
-->

> Stability: 1 - Experimental

* `path` {string|Buffer|URL}
* `options` {Object}
* `type` {string} An optional mime type for the blob.
* Return: {Promise} containing {Blob}

Returns a {Blob} whose data is backed by the given file.

The file must not be modified after the {Blob} is created. Any modifications
will cause reading the {Blob} data to fail with a `DOMException` error.
Synchronous stat operations on the file when the `Blob` is created, and before
each read in order to detect whether the file data has been modified on disk.

```mjs
import { openAsBlob } from 'node:fs';

const blob = await openAsBlob('the.file.txt');
const ab = await blob.arrayBuffer();
blob.stream();
```
* `callback` {Function}
* `err` {Error}
* `blob` {Blob}

```cjs
const { openAsBlob } = require('node:fs');

(async () => {
const blob = await openAsBlob('the.file.txt');
const ab = await blob.arrayBuffer();
blob.stream();
})();
```
For detailed information, see the documentation of the Promises version
of this API: [`fsPromises.openAsBlob()`][].

### `fs.opendir(path[, options], callback)`

Expand Down Expand Up @@ -5602,6 +5624,22 @@ this API: [`fs.mkdtemp()`][].
The optional `options` argument can be a string specifying an encoding, or an
object with an `encoding` property specifying the character encoding to use.

### `fs.openAsBlobSync(path[, options])`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

* `path` {string|Buffer|URL}
* `options` {Object}
* `type` {string} An optional mime type for the blob.
* Returns: {Blob}

For detailed information, see the documentation of the Promises version
of this API: [`fsPromises.openAsBlob()`][].

### `fs.opendirSync(path[, options])`

<!-- YAML
Expand Down Expand Up @@ -8149,6 +8187,7 @@ the file contents.
[`fsPromises.access()`]: #fspromisesaccesspath-mode
[`fsPromises.copyFile()`]: #fspromisescopyfilesrc-dest-mode
[`fsPromises.open()`]: #fspromisesopenpath-flags-mode
[`fsPromises.openAsBlob()`]: #fspromisesopenasblobpath-options
[`fsPromises.opendir()`]: #fspromisesopendirpath-options
[`fsPromises.rm()`]: #fspromisesrmpath-options
[`fsPromises.stat()`]: #fspromisesstatpath-options
Expand Down
48 changes: 40 additions & 8 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ const {
ObjectDefineProperties,
ObjectDefineProperty,
Promise,
PromiseResolve,
ReflectApply,
SafeMap,
SafeSet,
Expand Down Expand Up @@ -572,17 +571,49 @@ function openSync(path, flags, mode) {
* @param {{
* type?: string;
* }} [options]
* @returns {Promise<Blob>}
* @param {(
* err?: Error,
* blob?: Blob
* ) => any} callback
anonrig marked this conversation as resolved.
Show resolved Hide resolved
* @returns {void}
*/
function openAsBlob(path, options = kEmptyObject) {
function openAsBlob(path, options, callback) {
Copy link
Member

Choose a reason for hiding this comment

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

Help me understand - why do we actually need an async API with a callback for this if it's always called synchronously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to have it aligned on API level. Having a function that pretends (at least, for now) to be async is less weird that not having it in other APIs at all.

I would like to see this PR superseded or followed up with proper implementation and maybe switch from Blob to File, but judging from #45258 (comment) the former seems to be problematic.

Copy link
Member

Choose a reason for hiding this comment

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

I miss @addaleax 😍 , I think she was spot on and the impl shouldn't use blocking I/O here and return a promise/take a callback that's quite confusing IMO.

Additionally I think in the case of Blob we can probably return immediately and defer access to the file to the point someone actually tries to do something with the blob. That can be synchronous and perform no I/O and would arguably be a better API (since it makes no assumptions about the file in-between creating the Blob and accessing it)

Copy link

@jimmywarting jimmywarting Sep 24, 2023

Choose a reason for hiding this comment

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

defer access to the file to the point someone actually tries to do something with the blob.

I think it's highly likely that they will do something with the blob if they have called openAsBlob
i think it should check if the file exist and throw a DOMexception NotFoundError otherwise.
it's not a case about if but when they will do any I/O operation after having called openAsBlob. Developers are most likely going to use it at some point. So why not just do it upfront right away rather then later? it's going to do all this work anyway

It should also guard against modifications afterwards.

const blob = openAsBlob(path)
fs.appendFileSync(path, data)
blob.text() // ups, should throw DOMException, detected mtime or size changes. 

I believe browser do also support renaming the file and moving it to another folder and still being readable afterwards. think i remember testing this a long time ago but can't remember if that was the case.

const blob = openAsBlob(path)
fs.moveSync(path, dest)
blob.text() // works

so i don't think the type, size, filename and lastModified date should be changeable after having called openAsBlob so that would involve having to stat the file and look up some properties for it.

I don't think it should be possible to call openAsBlob(path) on a path that dose not exist, create the file and then do a blob.text() afterwards. that would just be weird and unexpected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda see the point of deferring the reading operations: the methods that allow reading content like Blob.prototype.arrayBuffer, Blob.prototype.text are async, so we only need stats to be determined immediately.
In case if I plan to read it via Blob.prototype.stream, I would be surprised to see that Node.js swallows whole file and keeps it in memory before even constructing the stream. After all, it's called openAsBlob and not readAsBlob.

Copy link

@jimmywarting jimmywarting Sep 24, 2023

Choose a reason for hiding this comment

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

After all, it's called openAsBlob and not readAsBlob.

Yup! Blob/File are more like FileSystemFileHandle or fs.FileHandle that just points to somewhere where it can locate and read the data from. (and what start/end offset it has)

a blob don't read anything to memory (or at least it should not - unless it's constructed from memory)

if (callback === undefined) {
callback = options;
options = kEmptyObject;
} else {
options ??= kEmptyObject;
}

validateObject(options, 'options');
const type = options.type || '';
validateString(type, 'options.type');
// The underlying implementation here returns the Blob synchronously for now.
// To give ourselves flexibility to maybe return the Blob asynchronously,
// this API returns a Promise.
path = getValidatedPath(path);
return PromiseResolve(createBlobFromFilePath(pathModule.toNamespacedPath(path), { type }));
anonrig marked this conversation as resolved.
Show resolved Hide resolved
validateFunction(callback, 'cb');

let blob;
let cbError = null;
try {
blob = createBlobFromFilePath(pathModule.toNamespacedPath(path), { type });
} catch (err) {
// Permission errors should be thrown instead of being passed to callback
if (err.code === 'ERR_ACCESS_DENIED') {
throw err;
}
cbError = err;
}

process.nextTick(callback, cbError, blob);
}

/**
* @param {string | Buffer | URL } path
* @param {{
* type?: string;
* }} [options]
* @returns {Blob}
*/
function openAsBlobSync(path, options) {
return syncFs.openAsBlob(path, options);
}

/**
Expand Down Expand Up @@ -3093,6 +3124,7 @@ module.exports = fs = {
open,
openSync,
openAsBlob,
openAsBlobSync,
readdir,
readdirSync,
read,
Expand Down
10 changes: 10 additions & 0 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ const { kFSWatchStart, watch } = require('internal/fs/watchers');
const nonNativeWatcher = require('internal/fs/recursive_watch');
const { isIterable } = require('internal/streams/utils');
const assert = require('internal/assert');
const { createBlobFromFilePath } = require('internal/blob');

const kHandle = Symbol('kHandle');
const kFd = Symbol('kFd');
Expand Down Expand Up @@ -756,6 +757,14 @@ async function fsync(handle) {
return binding.fsync(handle.fd, kUsePromises);
}

async function openAsBlob(path, options = kEmptyObject) {
validateObject(options, 'options');
const type = options.type || '';
validateString(type, 'options.type');
path = getValidatedPath(path);
return createBlobFromFilePath(pathModule.toNamespacedPath(path), { type });
}

async function mkdir(path, options) {
if (typeof options === 'number' || typeof options === 'string') {
options = { mode: options };
Expand Down Expand Up @@ -1075,6 +1084,7 @@ module.exports = {
opendir: promisify(opendir),
rename,
truncate,
openAsBlob,
rm,
rmdir,
mkdir,
Expand Down
17 changes: 16 additions & 1 deletion lib/internal/fs/sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ const {
getStatFsFromBinding,
getValidatedFd,
} = require('internal/fs/utils');
const { parseFileMode } = require('internal/validators');
const {
parseFileMode,
validateObject,
validateString,
} = require('internal/validators');
const { kEmptyObject } = require('internal/util');
const { createBlobFromFilePath } = require('internal/blob');

const binding = internalBinding('fs');

Expand Down Expand Up @@ -51,6 +57,14 @@ function copyFile(src, dest, mode) {
);
}

function openAsBlob(path, options = kEmptyObject) {
validateObject(options, 'options');
const type = options.type || '';
validateString(type, 'options.type');
path = getValidatedPath(path);
return createBlobFromFilePath(pathModule.toNamespacedPath(path), { type });
}

function stat(path, options = { bigint: false, throwIfNoEntry: true }) {
path = getValidatedPath(path);
const stats = binding.statSync(
Expand Down Expand Up @@ -91,6 +105,7 @@ module.exports = {
exists,
access,
copyFile,
openAsBlob,
stat,
statfs,
open,
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/permission/fs-read.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ const regularFile = __filename;
// fs.openAsBlob
{
assert.throws(() => {
fs.openAsBlob(blockedFile);
fs.openAsBlob(blockedFile, () => {});
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemRead',
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-blob-file-backed.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const {
const { TextDecoder } = require('util');
const {
writeFileSync,
openAsBlob,
promises: { openAsBlob },
} = require('fs');

const {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-permission-fs-supported.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const supportedApis = [
...syncAndAsyncAPI('mkdir'),
...syncAndAsyncAPI('mkdtemp'),
...syncAndAsyncAPI('open'),
'openAsBlob',
...syncAndAsyncAPI('openAsBlob'),
...syncAndAsyncAPI('mkdtemp'),
...syncAndAsyncAPI('readdir'),
...syncAndAsyncAPI('readFile'),
Expand Down