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 write methods param validation and docs #41677

Merged
merged 14 commits into from
Apr 4, 2022

Conversation

LiviaMedeiros
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros commented Jan 24, 2022

Refs: #41666

Potentially breaking changes.

An attempt to implement "named parameters" for filehandle.write (Promises API) and fs.writeSync (Synchronous API).

  1. Segregated ArrayBufferView and string forms in documentation.
    Current documentation, also including fs.write (Callback API), described behaviour for string|Object argument in [buffer, offset, length, position] version.
  2. Removed Objects with own .toString functions from accepted parameters.
    This functionality doesn't help with most cases, when an object inherits .toString from its prototype
    This functionality doesn't work with Symbol.toPrimitive
    This functionality is easily replaceable with obj.toString(), String(obj) or ""+obj
    This functionality in Promises and Synchronous APIs seems to be broken for a long time
  3. Function signatures are chosen to match their .read counterparts.
    Promises API: buffer, offset, length, position => options
    Synchronous API: buffer, offset, length, position => buffer, options

At this point, these changes should not break any existing working code.
For code trying to use documented feature of accepting objects with explicit .toString, outcome changes from abortion to exception.

Any feedback is appreciated.

Edit [2022-02-26]:
Soft-blocked by #42128
Default value of length is being changed for reading methods: it should be aligned for writing methods, and it would require adjustments in tests here.

Edit [2022-02-27]:
WIP, adjusting default length and tests.
done

Edit [2022-04-04]:
This PR doesn't add new feature anymore (847749f)
Optional params will be added in separate PR.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jan 24, 2022
@benjamingr
Copy link
Member

@nodejs/fs

@benjamingr benjamingr added needs-citgm PRs that need a CITGM CI run. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 24, 2022
@LiviaMedeiros
Copy link
Contributor Author

Added tests, covered somewhat related edge cases in existing tests, rebased on master branch.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

We should add entries to the changes list in the YAML comment to say that objects with an own toString method are no longer supported with this change.

@LiviaMedeiros
Copy link
Contributor Author

Sure, added to changes via 6f1344b

This PR makes separation between .write(string, ...) and .write(buffer, ...) in the docs stronger, so I've added it only in string version; while previous history stays reflected in both.
Does it look correct, or should any other entries be added or removed?

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@mmarchini
Copy link
Contributor

cc @nodejs/tsc since it's a semver-major

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@LiviaMedeiros
Copy link
Contributor Author

I would like to hear any thoughts about fs.write(fd, string[, position[, encoding]], callback) from Callback API with typeof string === 'object', and its future.

Aside from issues mentioned earlier, the current implicit object stringifying has two more.

  • Symbol.toPrimitive (even if inherited) has "higher priority" over toString (even if own).
    Therefore, this method might do a weird thing: asking for explicit own .toString, but using inherited @@toPrimitive instead.
    Worst case scenario: toPrimitive cast added to ancestor object, with different logic than target's toString.
  • Third param of the callback function called string and defined as string by documentation is actually not a string but the original object, i.e. string param passed in fs.write.

Some ugly code showcasing the problematic behaviour:

import * as fs from 'fs';
const OUTFILE = '/tmp/fswriteobj';

function TstCtor(data) {
  this._data = data;
  this._json = JSON.stringify(data);
  this[Symbol.toPrimitive] = () => this._data?.length;
  this.toString = () => this._json;
}

class TstClass {
  [Symbol.toPrimitive]() { return 'P' };
  toString() { return 'S' };
}

const tst1 = {toString: () => tst1.undefinedProperty};
const tst2 = {toString: () => 'S', [Symbol.toPrimitive]: () => 'P'};

const tst3 = new String('A');
const tst4 = new String('A');
  tst4[Symbol.toPrimitive] = () => 'P';
const tst5 = new String('A');
  tst5.toString = () => 'S';
const tst6 = new String('A');
  tst6[Symbol.toPrimitive] = () => 'P';
  tst6.toString = () => 'S';

const tst7 = new TstCtor(["one","two"]);

const tst8 = new TstClass();
const tst9 = new TstClass();
  tst9.toString = () => 'S';

// "undefined", "P", TypeError, TypeError, "S", "P", "2", TypeError, "P"
fs.open(OUTFILE, 'w', (err, fd) => {
  for (const writeMe of [tst1,tst2,tst3,tst4,tst5,tst6,tst7,tst8,tst9]) {
    try {
      fs.write(fd, writeMe, (err, written, string) => console.log(typeof string));
    } catch(e) {
      console.error(e.name);
    }
  }
});

It also shows that toString defined in class doesn't allow typecasting here, since it's not an ownProperty even on direct class instance.

Of course, the second issue can be fixed in docs; and there are several trivial ways to "fix" first one, e.g. by

- const str = String(buffer);
+ const str = String(buffer.toString());

but I don't see any really good solution. It's either breaking changes in API anyway, or documenting on how and why fs.write(fd, stringableObj, ...), fs.write(fd, String(stringableObj), ...) and fs.write(fd, stringableObj.toString(), ...) might produce different results. Or both.

Am I missing something, or it deserves to become deprecated for now, in favor of fs.write(fd, options, callback) form later?

@aduh95
Copy link
Contributor

aduh95 commented Feb 13, 2022

  • Symbol.toPrimitive (even if inherited) has "higher priority" over toString (even if own).
    Therefore, this method might do a weird thing: asking for explicit own .toString, but using inherited @@toPrimitive instead.

Good catch, I think this have been overlooked in the original implementation (own toString check was added in #34993). Calling toString seems like a good first workaround that can be backported without breaking changes to current release lines.

A deprecation also makes sense to me, I think asking folks to wrap their object in String() before passing it to fs.write is an OK tradeoff.

@LiviaMedeiros
Copy link
Contributor Author

Thanks; I'm leaning towards deprecation for the following reasons:

  • I don't see any workaround that will provide reasonable results with strict, obvious and laconic rules.
    In any code where result depends on Symbol and meeting the "has own toString" requirement was unintended or obsoleted, things will break. And it won't be fun to debug, since objectToWrite === stringPassedToCallback && String(objectToWrite) === String(stringPassedToCallback) && String(objectToWrite) === expectedResult.
    Also it will introduce a weird edge case: technically, .toString is allowed to return any primitive type. So we'll have to cast it (which means, objectToWrite.toString() is not guaranteed to be equivalent to what was written).
    Any other rule combination, such as allowing own @@toPrimitive to be an alternative, would also pollute the code, have complicated docs and potentially break someone's code.
  • options argument as in this PR is supported by Callback/fs.read, so it would be natural to implement it for fs.write later.
  • The mentioned PR was originally based on supporting already deprecated uglify-js v2 and IE8.

Thereby, I'd rather not fix it and definitely not backport, unless there's some software that actually relies on this feature, actually suffers from "bug" in it, and for some reason can't be fixed by explicit typecast.
Legacy code usually wants API to be remain unchanged no matter what.

Since this PR is semver-major and code-wise changes will be the same as for promises and sync versions, could it be done there? Or I should open a standalone deprecation PR?

This was referenced Apr 5, 2022
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol
Copy link
Member

This should not be landed in v17.x, ref: #42613 (comment)

@aduh95
Copy link
Contributor

aduh95 commented Apr 6, 2022

This should not be landed in v17.x, ref: #42613 (comment)

@juanarbol that’s incorrect, this should land on v17.x.

aduh95 pushed a commit to aduh95/node that referenced this pull request Apr 6, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: nodejs#34993
PR-URL: nodejs#41677
Refs: nodejs#41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Apr 7, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Backport-PR-URL: #42631
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Apr 21, 2022
This also affects `fs.writeFile`, `fs.appendFile`, and
`fs.appendFileSync`

Refs: #41677

PR-URL: #42149
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
This also affects `fs.writeFile`, `fs.appendFile`, and
`fs.appendFileSync`

Refs: #41677

PR-URL: #42149
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
This also affects `fs.writeFile`, `fs.appendFile`, and
`fs.appendFileSync`

Refs: #41677

PR-URL: #42149
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
This also affects `fs.writeFile`, `fs.appendFile`, and
`fs.appendFileSync`

Refs: #41677

PR-URL: #42149
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
This also affects `fs.writeFile`, `fs.appendFile`, and
`fs.appendFileSync`

Refs: nodejs#41677

PR-URL: nodejs#42149
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
The FS docs wrongfully indicated support for passing object with an own 
`toString` function property to `FileHandle.prototype.appendFile`, 
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, 
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and 
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object 
was passed to `FileHandle.prototype.write`.

Refs: nodejs#34993
PR-URL: nodejs#41677
Refs: nodejs#41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request May 1, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: #42603
@juanarbol juanarbol mentioned this pull request May 1, 2022
@juanarbol juanarbol removed the needs-ci PRs that need a full CI run. label May 29, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Refs: #41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: nodejs/node#34993
PR-URL: nodejs/node#41677
Refs: nodejs/node#41666
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants