test: add common.mustNotMutateObjectDeep()

[LiviaMedeiros]

Problem

tl;dr: usually we don't want core methods to have side effects of mutating input values.

Let's imagine function having the following flow inside:

function makeRequest(ipaddr, options) {
  if (typeof options.port === 'number')
    return _mkRq(ipaddr, options);
  switch (options.protocol) {
    case 'ftp': options.port = 21; break;
    case 'http': options.port = 80; break;
    default: // unknown, whatever
  }
  return _mkRq(ipaddr, options);
}

Let's try to use it:

const opts = { headers };
makeRequest(addr, opts); // unknown://addr:unknown --- ok
opts.protocol = 'http';
makeRequest(addr, opts); // http://addr:80 --- got side effects
opts.protocol = 'ftp';
makeRequest(addr, opts); // ftp://addr:80 --- suffered from side effects

To run into such things in tests, we need to perform specific calls in specific order, and it still doesn't guarantee catching. Simple tests like makeRequest(addr, Object.freeze({})) won't help if mutation is conditional.

Solution

This PR adds common.mustNotMutate() which improves our ability to test functions for undesired side effects.

Pass an object into it and use returned value in tests whenever it must remain unchanged:

...
opts.protocol = 'http';
makeRequest(addr, common.mustNotMutate(opts)); // http://addr:80 --- testing for side effects

AssertionError [ERR_ASSERTION]: Expected no side effects, got 80 assigned to port

Or make an immutable "view" on any object:

const mFoo = { bar: 'baz' };
const iFoo = mustNotMutate(mFoo);
mFoo.bar = 'qux'; // affects BOTH mFoo and iFoo
iFoo.bar = 'gwak'; // AssertionError

Or keep mutable "view" and make an object immutable:

const realProcessEnv = process.env;
process.env = mustNotMutate(process.env);
realProcessEnv.LANG = 'en_GB'; // affects BOTH realProcessEnv and process.env
process.env.LANG = 'en_GB'; // AssertionError

Example

[WIP] Using this function in some fs tests: LiviaMedeiros@48a8bd1

[tniessen]

Where would we use this? There must be tens of thousands of function calls that pass options objects or other data structures that should not be mutated.

[LiviaMedeiros]

Utopically, everywhere - just like common.mustSucceed() wraps most callbacks. :)

Practically, in places where there is a reasonable chance that accidental side effects might happen. Mainly in new tests or refactoring old ones. A lot of options objects in fs, http* and crypto require additional testing for that.

Mandatory, whenever a fix for such issues is introduced. Example:

node/lib/https.js

Lines 114 to 131 in 304e06a

	function createConnection(port, host, options) {
	if (port !== null && typeof port === 'object') {
	options = port;
	} else if (host !== null && typeof host === 'object') {
	options = { ...host };
	} else if (options === null || typeof options !== 'object') {
	options = {};
	} else {
	options = { ...options };
	}
	if (typeof port === 'number') {
	options.port = port;
	}
	if (typeof host === 'string') {
	options.host = host;
	}

// options = { ...port };
// Regression test for https://github.com/nodejs/node/issues/31119
agent.createConnection(mustNotMutate({ port: 443 }), 'nodejs.org');

[F3n67u]

Where would we use this? There must be tens of thousands of function calls that pass options objects or other data structures that should not be mutated.

hi! @tniessen I am new to node source code. Could you help me to understand why there is so many side effect(mutation) to function parameter? I don't think it's a good practice to mutate function prameter.

[tniessen]

Utopically, everywhere - just like common.mustSucceed() wraps most callbacks. :)

Yeah, I wrote mustSucceed and IIRC not everyone liked the idea. Still, in that case, most of the modified sources only simplified mustCall(ifError()) to mustSucceed() and didn't require additions beyond that. Luckily, for the most part, that's somewhat legacy now, since async addresses the problem at the language level for new Promise APIs. In this case, the closest thing to a language-level solution might be proposal-record-tuple, unfortunately.

Don't get me wrong, I like having this guarantee, and ideally, we'd have some mechanism to detect these modifications. I am just not sure if I like the idea of bloating our test files with thousands of such checks to guard against this specific problem. I only remember very few issues that reported such side effects. And, because there is no good coverage metric for this, we'd probably have to wrap every argument in such a call, even arguments that aren't object literals, and then we'd still only be guarding against this specific side effect. (For example, we also don't check if functions modify any existing prototypes or built-in objects.)

(It might be less work to rewrite core in TypeScript and declare most inputs as deep-readonly objects.)

Mandatory, whenever a fix for such issues is introduced. Example:

For regression tests, I agree, this is a good idea, but in those cases, we can usually write a specific test for the specific mutation in one or two lines of code.

hi! @tniessen I am new to node source code. Could you help me to understand why there is so many side effect(mutation) to function parameter?

@F3n67u It's the opposite. We usually don't mutate function parameters, but we don't test that explicitly because it would require adding tens of thousands of checks.

[LiviaMedeiros]

In this case, the closest thing to a language-level solution might be proposal-record-tuple, unfortunately.

Yep, looking forward to this as well as proposal-type-annotations, and hoping for eventual deprecation of most callback-oriented APIs in Node.js; but at this moment we still need to deal with callbacks, dynamic typings, etc.

I am just not sure if I like the idea of bloating our test files with thousands of such checks to guard against this specific problem.

Sorry if I worded it wrong. I'm definitely not proposing rewriting even hundreds of test files with this method, even ideally. Only utopically, if we could magically have zero performance, readability and maintainability cost for that.

My current plan is to cover all most-likely-to-become-affected methods in fs with this approach, which is about ten files, ideally w/o creating new ones. After that, to check most-suspiciously-looking methods in other subsystems, which probably also is a few tens of files. This task alone would be significantly harder and uglier without a common helper method.

Aside from that, if people will find ability to use bulltin wrapper instead of writing specific tests or shaping specific test flows to make proper immutability testing possible, that's great. If anyone will integrate it in existing tests where this check is important, that's cool as well. Even if not, well, AFAICT it doesn't add any overhead to the node binary. :)

Regarding other types of side effects - maybe guarding against them makes sense, and maybe some of them are doable even as "automatic" job of common module. However, unless mustNotMutate() should play major role in it and requires some specific redesign, it's out of scope for this PR.
As for now, we can do some generic stuff using it, such as:

Math = mustNotMutate(Math);
Math.random = ()=>Number.EPSILON; // AssertionError

const mutableRealProcess = process;
process = mustNotMutate(process);
process.env.HOME = '/proc'; // AssertionError
mutableRealProcess.env.HOME = '/sys';
console.log(process.env.HOME); // /sys

[RaisinTen]

I think test/common is for utilities that are repeated across multiple tests. Are there occurrences of this testing pattern in places other than what we have in

node/test/parallel/test-https-agent-create-connection.js

Lines 152 to 154 in 6375b13

	assert.deepStrictEqual(options, {
	port: 3000, rejectUnauthorized: false
	});

? If so, I agree it would make sense to place this utility in here and use this in all of those places.

[LiviaMedeiros]

I think test/common is for utilities that are repeated across multiple tests. Are there occurrences of this testing pattern in places other than what we have in [...]? If so, I agree it would make sense to place this utility in here and use this in all of those places.

Tens of thousands of potential places. Covered (hopefully near-exhaustive) fs via 3da5607 as an example of wrapper pattern usage.

As for already-existing examples, I know about

1. fs: don't alter user provided options object #7831 (whole test-fs-options-immutable.js)
2. https: make opts optional & immutable when create #13599:

   node/test/parallel/test-https-argument-of-creating.js

   Lines 13 to 22 in 0818b52

   	// Test for immutable `opts`
   	{
   	const opts = { foo: 'bar', ALPNProtocols: [ 'http/1.1' ] };
   	const server = https.createServer(opts);
   	tls.convertALPNProtocols([ 'http/1.1' ], dftProtocol);
   	assert.deepStrictEqual(opts, { foo: 'bar', ALPNProtocols: [ 'http/1.1' ] });
   	assert.strictEqual(server.ALPNProtocols.compare(dftProtocol.ALPNProtocols),
   	0);
   	}

(1) one is good, but basically uses the same idea (making object immutable and then passing it everywhere) and doesn't cover possible conditional failures.
(2) is good as well, but I think const server = https.createServer(mustNotMutate({ foo: 'bar', ALPNProtocols: [ 'http/1.1' ] })); would allow to avoid surrounding boilerplate code, and that boilerplate code like this one is the main reason why immutability check is not widespread enough.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44197/

[RaisinTen]

Okay, so the known list of existing tests that uses this pattern is:

• node/test/parallel/test-https-agent-create-connection.js

  Lines 152 to 154 in 6375b13

  	assert.deepStrictEqual(options, {
  	port: 3000, rejectUnauthorized: false
  	});

• node/test/parallel/test-https-argument-of-creating.js

  Lines 13 to 22 in 0818b52

  	// Test for immutable `opts`
  	{
  	const opts = { foo: 'bar', ALPNProtocols: [ 'http/1.1' ] };
  	const server = https.createServer(opts);
  	tls.convertALPNProtocols([ 'http/1.1' ], dftProtocol);
  	assert.deepStrictEqual(opts, { foo: 'bar', ALPNProtocols: [ 'http/1.1' ] });
  	assert.strictEqual(server.ALPNProtocols.compare(dftProtocol.ALPNProtocols),
  	0);
  	}

• node/test/parallel/test-fs-options-immutable.js

  Line 12 in 15a682c

  const options = Object.freeze({});

I would suggest that we should limit the scope of this PR to adding common.mustNotMutate() + docs + changing only these ^ tests + the test/parallel/test-common-must-not-mutate.mjs test that you've already added.

The rest of the modifications in the current state of this PR starts using common.mustNotMutate() in places where input mutation wasn't actually tested before. I think that should be done in its own PR separate from this change.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44225/

[LiviaMedeiros]

Agreed, done.

[RaisinTen]

LGTM

[RaisinTen]

@nodejs/testing wdyt?

[ljharb]

Proxies aren’t transparent for builtins without a full membrane; i would expect this to cause all sorts of weird problems when used with any builtin object.

[LiviaMedeiros]

Proxies aren’t transparent for builtins without a full membrane; i would expect this to cause all sorts of weird problems when used with any builtin object.

Yep, that's fair. I didn't meet (yet?) any situation in tests where this function was applicable to an object with unproxyable internal slots, but it's definitely worth a caveat for now, and a workaround if usecases will surface.

Is there a list of such builtins to put a reference?

[ljharb]

Everything in JavaScript - arrays, promises, dates, regexes, maps, sets, errors, etc.

[ljharb]

@LiviaMedeiros to add more context, there's a reason no test framework in JS that i'm aware of has this kind of helper - Proxies, absent a full membrane, simply aren't generic stand-ins for JS objects. In particular, Maps and Sets can't be generically made immutable (you can hardcode knowledge of all of the builtin functions, but Map.prototype.add.call would throw because a Proxy for a Map would lack the internal slots of a Map). Objects can be, with Object.freeze, but that's itself a mutation, and one that can't be cleaned up/reversed later. This also applies to virtually all of node core, since node core uses the primordials pattern, and thus doesn't typically access members of objects.

This also applies to any userland object that uses identity in a closed-over collection, or private fields, since the Proxy for the instance wouldn't have the same identity as the instance itself.

I think this kind of helper is simply inappropriate to build into core, or into any testing framework - the way to ensure no mutations is to snapshot in some way the object before and after, and compare the snapshots.

[LiviaMedeiros]

This function is intended to be used only within Node.js tests, i.e. it won't be a part of public API or a part of underlying core. It is not intended to be an example for testing frameworks; if there is a high chance that the code would be used in more generic testing, we can add more warnings in docs or even code comments later. I'm open for suggestions on how it should be documented to avoid any confusion or pitfalls.
If there will be high chance of misusing in Node.js tests, we can add workarounds when this function gets instanceof Map || ....

Right now I don't see any considerable amount of places where problematic builtins or private fields are expected to be used, but I see a lot of places where we have predefined Objects that need this testing and from my understanding are completely safe for being wrapped into Proxy.

On top of that, I don't see a huge problem in methods throwing on Proxy in this context. Yes, things like new Proxy(new Map,{}).set('foo', 'bar') or new Proxy(new Set,{}).add('baz') will throw TypeError. We do want them to throw, because we don't want them to be called. In fact, we do want non-AssertionError and right now we do want throwing new Proxy(new Map,{}).has('foo') as well, because it would help preventing situation where someone expects this to work. :)
For typical usecase when we pass completely arbitrary garbage to test for ERR_INVALID_ARG_TYPE, it seems to fit perfectly as well.

Regarding alternative way of snapshoting and comparing, there are at least two major issues:

1. It is verbose, it is inconvenient (depending on what and how we test), it bloats the code. Adding this function to most of applicable tests in most of subsystems is achievable. Adding same amount of testing with "traditional" way - I doubt. Asking people to wrap some objects into common function in new tests sounds okay, asking to add seemingly-unrelated boilerplate code or separate tests for that - not really.
2. This approach is not failproof. For example, what if a method makes internal copy, mutates nested object and in the end replaces everything back? Wouldn't it remain unnoticed until someone's asynchronous code goes crazy?

[ljharb]

I agree that a snapshotting approach isn’t viable for the reasons you indicate - it’s that i think this isn’t a solvable problem in a practical sense at all.

[tniessen]

I'd prefer the documentation to include a guideline, maybe something along the lines Use of this function is encouraged only for relevant regression tests.

[aduh95]

nits:

[aduh95]

Suggested change

	// Make a copy to make sure original doesn't get altered by the function itself
	// Make a copy to make sure original doesn't get altered by the function itself.

[LiviaMedeiros]

Is there a stlyguide-ish reference for that? All the comments are grouped with corresponding code lines by having empty lines, so I'm not sure if punctuation helps.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45374/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/43196
✔  Done loading data for nodejs/node/pull/43196
----------------------------------- PR info ------------------------------------
Title      test: add `common.mustNotMutateObjectDeep()` (#43196)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     LiviaMedeiros:ft-must-not-mutate -> nodejs:main
Labels     test, author ready, needs-ci, commit-queue-rebase
Commits    2
 - test: add `common.mustNotMutateObjectDeep()`
 - test: use `common.mustNotMutateObjectDeep()` in immutability tests
Committers 1
 - LiviaMedeiros 
PR-URL: https://github.com/nodejs/node/pull/43196
Reviewed-By: Darshan Sen 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43196
Reviewed-By: Darshan Sen 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - test: add `common.mustNotMutateObjectDeep()`
   ⚠  - test: use `common.mustNotMutateObjectDeep()` in immutability tests
   ℹ  This PR was created on Tue, 24 May 2022 12:23:10 GMT
   ✔  Approvals: 2
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/43196#pullrequestreview-990592889
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43196#pullrequestreview-1036243936
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-07-12T21:56:39Z: https://ci.nodejs.org/job/node-test-pull-request/45374/
- Querying data for job/node-test-pull-request/45374/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2663402216

[LiviaMedeiros]

Landed in 99b109f...0c3b00e