Feature/set #2961

AzatCoder · 2022-07-19T11:41:25Z

Hi there!
Let's add function set to underscore.
#2948

jgonggrijp

Thank you for contributing!

I want to merge your branch, but it needs a bit more work. I suggest you work in the following order:

I requested a change in the tests (last comment). Please make that change before changing the logic and verify that it fails initially.
I pointed out three logical mistakes in my comments below: isArray/isObject in _set.js:14, isNumber in _set.js:16 and _.toPath in set.js:8. Before addressing them, please add tests that expose them (i.e., that fail initially).
Your code is currently vulnerable to prototype pollution. Add a test that demonstrates this.
Make the edited tests pass and address the remainder of my comments, possibly taking inspiration from the code I suggested in Possibility to update a nested object's property in a non-mutable way #2948.
Add an entry to the index.html to document the new set function. (I know the CONTRIBUTING.md states you shouldn't update the documentation. That is meant to refer only to the annotated sources and the changelog, both of which are updated on release. You are welcome to clarify that part of the CONTRIBUTING.md if you feel like it.)

jgonggrijp · 2022-07-19T12:58:28Z

modules/_set.js

+import isObject from './isObject.js';
+
+
+export default function set (obj, path, value) {


Three changes needed for this internal function as a whole:

It is not used anywhere except in the public set, so it does not need to be in a separate module (compare isEqual). Please move it into ./set.js. You can call it deepSet (cf. deepGet).

Please add a top comment with a description, like all other functions.

(minor) The general convention in Underscore is to have no space between the function name and the parameter list, so please remove it.

Suggested change

export default function set (obj, path, value) {

export default function set(obj, path, value) {

jgonggrijp · 2022-07-19T12:59:23Z

modules/_set.js

+
+
+export default function set (obj, path, value) {
+  var key = String(path[0]);


I don't think calling String adds any value here.

jgonggrijp · 2022-07-19T13:34:41Z

modules/_set.js

+    return;
+  }
+
+  if (!isArray(obj[key]) || !isObject(obj[key])) {


This condition doesn't quite do what you want. Lifting the negation, you get !(isArray(obj[key]) && isObject(obj[key])). However, isArray(obj) implies isObject(obj), so this entire expression reduces to just !isArray(obj[key]). In other words, this block is executed if obj[key] is any non-array value, even if it is an object.

yep, my bad. It should be if (!isObject(obj[key])) {...}

jgonggrijp · 2022-07-19T13:36:31Z

modules/_set.js

+
+  if (!isArray(obj[key]) || !isObject(obj[key])) {
+    var nextKey = path[1];
+    obj[key] = isNumber(nextKey) ? [] : {};


isNumber('123') returns false, which is not what you want in this case. You need to match nextKey against a regular expression, as I demonstrated in #2948.

jgonggrijp · 2022-07-19T13:37:31Z

modules/set.js

+import _set from './_set.js';
+
+
+// set the value in given path


Underscore is literate code. Please elaborate a bit and write full sentences.

jgonggrijp · 2022-07-19T13:37:46Z

modules/set.js

+
+
+// set the value in given path
+export default function set (obj, path, value) {


Suggested change

export default function set (obj, path, value) {

export default function set(obj, path, value) {

jgonggrijp · 2022-07-19T13:40:37Z

modules/set.js

+
+// set the value in given path
+export default function set (obj, path, value) {
+  if (!isObject(obj) || !isArray(path)) return obj;


By convention, Underscore functions allow you to pass a single path component as a bare string or number instead of an array. Take path through _.toPath instead of rejecting it if it is not an array.

jgonggrijp · 2022-07-19T13:55:41Z

modules/set.js

+// set the value in given path
+export default function set (obj, path, value) {
+  if (!isObject(obj) || !isArray(path)) return obj;
+  if (path.length === 0) return obj;


You're checking path.length here and then again in the internal set function. Not a big deal, but slightly inefficient.

Sorry, I don't see other option better than this. I like this implementation. Please let me know if you have an idea how to improve it.

Please refer to the approach I suggested in #2948. If you do postorder assignment instead of preorder assignment, you can check for path.length === 0 only in the internal function and immediately return value if true. You can avoid lookahead beyond path[0] in that case by replacing obj instead of obj[key].

Referring again to #2948, I also realize that your code is still vulnerable to prototype pollution. You need to fix that as well.

jgonggrijp · 2022-07-19T13:56:13Z

test/objects.js

+    assert.strictEqual(_.set(undefined, ['some', 'path'], 'some value'), undefined);
+    assert.strictEqual(_.set(null, ['some', 'path'], 'some value'), null);
+    assert.strictEqual(_.set(12, ['some', 'path'], 'some value'), 12);
+    assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));


We are trying to support old environments, so you cannot just sprinkle post-ES3 features in the tests (or the source code, for that matter). To fix, make this line conditional on feature detection:

Suggested change

assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));

if (typeof BigInt != 'undefined') {

assert.strictEqual(_.set(BigInt(1), ['some', 'path'], 'some value'), BigInt(1));

}

You'll find similar code elsewhere in this module.

jgonggrijp · 2022-07-19T13:56:54Z

test/objects.js

+    assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
+    assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': {'0': 'my value'}});


I disagree with this difference. Numeric keys are implicitly converted to strings. Array indices are actually strings, too. The second test should generate the same value as the first.

Suggested change

assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});

assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': {'0': 'my value'}});

assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});

assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': ['my value']});

AzatCoder · 2022-07-19T17:23:38Z

Thank you for quick reviewing, @jgonggrijp

coveralls · 2022-07-22T16:35:39Z

Coverage increased (+0.08%) to 95.299% when pulling b5f481a on AzatCoder:feature/set into a15d1af on jashkenas:master.

e.g. you can do setFieldValue('replicator.1.foo', 'bar') to update the foo field on the second replicator set to bar This uses the eventual underscore 'set' method from jashkenas/underscore#2961. Once it's merged we can call the method directly and not need to include these files.

jgonggrijp

I'm quite happy! I just think we should avoid using _.contains for this particular function, to keep the treeshakers happy, and add two more safeguards against prototype pollution.

Nearly there!

jgonggrijp · 2022-07-25T12:33:16Z

modules/set.js

+  path = toPath(path);
+
+  if (!isObject(obj) || !path.length) return obj;
+  if (contains(path, '__proto__')) throw new Error('Prototype assignment attempted');


Two things. Firstly, I'm in favor of reusing our own function in principle. However, in this case the use of _.contains means that _.get will come out a bit "fatter" when people try to treeshake it. For this reason, I suggest checking the current key inside deepGet instead.

Secondly, I just realized we need to check not only for __proto__ but also for constructor and prototype. Otherwise, attackers can (for example) still pollute String methods.

jgonggrijp · 2022-07-25T12:38:36Z

modules/set.js

+// Internal function of `set`.
+function deepSet(obj, path, value) {
+  var key = path[0];
+


I think this is the right place to check that key is not one of '__proto__', 'constructor' or 'prototype'.

I'm afraid it will mutate before throwing an error.

const obj = { value: 50 }; try { _.set(obj, [ 'key', '__proto__' ], {}); } catch(e) { console.log(e.message); // Prototype assignment attempted console.log(obj); // { value: 50, key: {} } - `obj` was mutated :( }

Yeah, I realized that as well. I can think of three options (from least to most preferred, in my opinion):

Stick with _.contains, accept the double iteration and the greater weight after treeshaking.

Accept that obj gets somewhat corrupted. At least the prototype pollution attempt is detected and unsuccessful.

Switch to postorder assignment as I suggested before, which has neither of these problems.

I didn't mention it because I was ready to accept option 2, but I would prefer option 3.

AzatCoder added 3 commits July 18, 2022 21:46

added new method set

23e2515

added tests for set

2e38b16

little comment for set

891f79a

jgonggrijp requested changes Jul 19, 2022

View reviewed changes

AzatCoder added 2 commits July 22, 2022 20:12

review

7c6c6a1

documentation

b5f481a

AzatCoder requested a review from jgonggrijp July 22, 2022 16:34

jgonggrijp requested changes Jul 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/set #2961

Feature/set #2961

AzatCoder commented Jul 19, 2022

jgonggrijp left a comment •

edited

jgonggrijp Jul 19, 2022

jgonggrijp Jul 19, 2022

jgonggrijp Jul 19, 2022

AzatCoder Jul 19, 2022

jgonggrijp Jul 19, 2022

jgonggrijp Jul 19, 2022

jgonggrijp Jul 19, 2022

jgonggrijp Jul 19, 2022

jgonggrijp Jul 19, 2022

AzatCoder Jul 19, 2022

jgonggrijp Jul 20, 2022 •

edited

jgonggrijp Jul 19, 2022

jgonggrijp Jul 19, 2022

AzatCoder commented Jul 19, 2022

coveralls commented Jul 22, 2022 •

edited

jgonggrijp left a comment

jgonggrijp Jul 25, 2022

jgonggrijp Jul 25, 2022

AzatCoder Jul 25, 2022

jgonggrijp Jul 25, 2022

		import isObject from './isObject.js';


		export default function set (obj, path, value) {

	export default function set (obj, path, value) {
	export default function set(obj, path, value) {



		export default function set (obj, path, value) {
		var key = String(path[0]);



		// set the value in given path
		export default function set (obj, path, value) {

		assert.deepEqual(_.set({x: 10}, [0, 0], 'my value'), {x: 10, '0': ['my value']});
		assert.deepEqual(_.set({x: 10}, [0, '0'], 'my value'), {x: 10, '0': {'0': 'my value'}});

Feature/set #2961

Are you sure you want to change the base?

Feature/set #2961

Conversation

AzatCoder commented Jul 19, 2022

jgonggrijp left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgonggrijp Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AzatCoder commented Jul 19, 2022

coveralls commented Jul 22, 2022 • edited

jgonggrijp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgonggrijp left a comment •

edited

jgonggrijp Jul 20, 2022 •

edited

coveralls commented Jul 22, 2022 •

edited