Skip to content
Draft
Show file tree
Hide file tree
Changes from 7 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
10 changes: 10 additions & 0 deletions src/expression/node/AccessorNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
isIndexNode,
isNode,
isObjectNode,
isOperatorNode,
isParenthesisNode,
isSymbolNode
} from '../../utils/is.js'
Expand Down Expand Up @@ -93,6 +94,15 @@ export const createAccessorNode = /* #__PURE__ */ factory(name, dependencies, ({
const evalObject = this.object._compile(math, argNames)
const evalIndex = this.index._compile(math, argNames)

// If index contains operator node, evaluate result of the operation and access object with result
if (isOperatorNode(this.index.dimensions[0]) && isObjectNode(this.object)) {
Copy link
Owner

Choose a reason for hiding this comment

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

This new section in AccessorNode adds support for operators inside the accessor like {2: 6}[2 * 1], but it doesn't allow any expression in general, for example it fails with {2: 6}[multiply(2, 1)].

I think the right place to update this behavior is not here in AccessorNode, but in IndexNode.isObjectProperty and IndexNode.getObjectProperty, see also my other comment in parse.js.

Copy link
Author

Choose a reason for hiding this comment

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

I'm having trouble updating this behaviour in IndexNode.getObjectProperty

I was able to compile the result of the operation because this was happening in a _compile method and I had access to math and argNames, so could call the compile method of the OperatorNode

In IndexNode.getObjectProperty, I don't have access to math and argNames, so can't use the _compile method on an OperatorNode in there. And I can't find another way to compile a result to the operation in an OperatorNode

Copy link
Owner

Choose a reason for hiding this comment

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

I've looked into this a bit more, it's more complex than I had expected.

In AccessorNode._compile, there is an if (this.index.isObjectProperty()) { ... }. I think that part of the code must be removed, since it only works for static strings and not expressions inside the brackets. And also, we cannot detect anymore whether we're dealing with an object key (vs a matrix subset) by just looking at the index: when the index now contains just one dimension with one number, it could be an object key too. Therefore, this should be handled by the access(...) function. The logic in the function access that handles the case of an object can be extended to correctly evaluate numeric properties too. To do that, instead of the index.getObjectProperty() we need a method like index.evalObjectProperty(scope, args, context). We cannot use the normal IndexNode._compile(...) because that changes numeric indexes from 1-based to 0-based, and we don't want that in case of an object key.

Similarly, we need to update/extend the logic in AssignmentNode.

Maybe we should also get rid of the "smart" name function of AccessorNode, see get name() { ... }. It's not really needed in practice I think, and is not reliable anymore when it only works in "some" cases.

Does this direction sort of make sense?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it all make sense. Thank you for the directions. I will try to work it out and revert

const operatorNode = this.index.dimensions[0]
return function evalAccessorNode (scope, args, context) {
const result = operatorNode._compile(math, argNames)(scope, args, context)
return getSafeProperty(evalObject(scope, args, context), String(result))
}
}

if (this.index.isObjectProperty()) {
const prop = this.index.getObjectProperty()
return function evalAccessorNode (scope, args, context) {
Expand Down
14 changes: 10 additions & 4 deletions src/expression/parse.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { factory } from '../utils/factory.js'
import { isAccessorNode, isConstantNode, isFunctionNode, isOperatorNode, isSymbolNode, rule2Node } from '../utils/is.js'
import { isAccessorNode, isConstantNode, isFunctionNode, isObjectNode, isOperatorNode, isSymbolNode, rule2Node } from '../utils/is.js'
import { deepMap } from '../utils/collection.js'
import { safeNumberType } from '../utils/number.js'
import { hasOwnProperty } from '../utils/object.js'
Expand Down Expand Up @@ -1413,6 +1413,12 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
closeParams(state)
getToken(state)

// If param value is number and node is object node, make param value a string
if (typeof params[0].value === 'number' && isObjectNode(node) && params.length === 1 && isConstantNode(params[0])) {
// Number constructor is first used to manage situations of numbers with preceding zero digit(s)
params[0].value = String(Number(params[0].value))
Copy link
Owner

Choose a reason for hiding this comment

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

Since it can be the case that the actual (numeric or string) key is evaluated at runtime and not parse time, I think this logic should not be located here but when evaluating the actual key, in functions like Indexnode.isObjectProperty and IndexNode.getObjectProperty. However, these functions currently determine whether dealing with an object node statically. I think this needs to be changed to determine this after evaluating it.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I will look into effecting the change from IndexNode instead

Copy link
Author

Choose a reason for hiding this comment

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

You're right...making the changes in IndexNode will affect assignments too, which is something I overlooked.

I'm thinking of introducing an optional property like 'forObjectNode'. But since there's already an optional property for dot notation, I'm now thinking of having an optional options object that would contain dotNotation and forObjectNode.

Copy link
Author

Choose a reason for hiding this comment

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

I've been able to mostly move this logic to IndexNode.isObjectProperty and .getObjectProperty

I did attach a boolean property, 'forObjectNode' to params[0] here

I opted for this against modifying the structure of arguments to the IndexNode constructor as I initially intended in my earlier comment, because this felt more controlled and still got the job done

However, using that forObjectNode property I attached to the nodes being fed into the IndexNode constructor, I've been able to recreate this logic you commented on here in IndexNode's isObjectProperty and getObjectProperty

Copy link
Author

Choose a reason for hiding this comment

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

@josdejong Can you please review this update?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it makes sense to await reviewing until you've tried out the latest ideas, or is there a specific part of the code on which you already would like to have more feedback?

Copy link
Author

Choose a reason for hiding this comment

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

@josdejong I've pushed my latest changes and I would like your review.

The earlier logic I had in parseAccessor has been completely moved out.
IndexNode.isObjectProperty and getObjectProperty have been restored to only recognize and return strings like before

To be able to handle numbers, operations and expressions in matrix indexes for object accessing and assignments, I relied on a singular DenseMatrix being present in the index._dimensions array recieved in the access and assign functions

Problems are that

  1. Somewhere in the Index transform process, it passes through the _createImmutableMatrix method in MatrixIndex and throws an error if non-integers are used
  2. The tests for getting names accurately for AccessorNode, AssignmentNode, FunctionNode are failing
  3. A few security tests related to not allowing calling Functions via various things (bind, constructor, etc) are failing...however they're failing because the errors gotten now don't exactly match the errors expected. But trying out the test cases still results in an error being thrown

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the updates, I'll do some testing and debugging with the latest version of your PR soon (I expect tomorrow).

It can indeed be that some of the security tests now give differing error messages, that is fine.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you very much

Copy link
Owner

Choose a reason for hiding this comment

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

About (2): the tests for names will need to change if we drop support for these "smart" names. It will be a breaking change.

Thinking about it however, it may be better to leave it as it is for now and mark it deprecated, then we don't introduce a breaking change right now, and it may be better to try focus on getting numbers working as index, which is complex enough already. Let's try one step at a time 😅 .

}

node = new AccessorNode(node, new IndexNode(params))
} else {
// dot notation like variable.prop
Expand Down Expand Up @@ -1615,11 +1621,11 @@ export const createParse = /* #__PURE__ */ factory(name, dependencies, ({
// parse key
if (state.token === '"' || state.token === "'") {
key = parseStringToken(state, state.token)
} else if (state.tokenType === TOKENTYPE.SYMBOL || (state.tokenType === TOKENTYPE.DELIMITER && state.token in NAMED_DELIMITERS)) {
key = state.token
} else if (state.tokenType === TOKENTYPE.SYMBOL || (state.tokenType === TOKENTYPE.DELIMITER && state.token in NAMED_DELIMITERS) || state.tokenType === TOKENTYPE.NUMBER) {
key = state.tokenType === TOKENTYPE.NUMBER ? String(Number(state.token)) : state.token
getToken(state)
} else {
throw createSyntaxError(state, 'Symbol or string expected as object key')
throw createSyntaxError(state, 'Symbol, numeric literal or string expected as object key')
}

// parse key/value separator
Expand Down
36 changes: 36 additions & 0 deletions test/unit-tests/expression/parse.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,42 @@ describe('parse', function () {
}, /Cannot apply a numeric index as object property/)
})

it('should coerce numbers to string when trying to apply a numeric key in an object expression', function () {
assert.deepStrictEqual(parseAndEval('{2: 6}'), { 2: 6 })
})

it('should throw an error when negative numbers are applied as keys in an object expression', function () {
assert.throws(function () {
parseAndEval('{-1: 34}')
Copy link
Owner

Choose a reason for hiding this comment

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

I agree with Glen: I cannot come up with a reason why we wouldn't support negative numbers. Can you implement support for negative values too? Or do you have an other opinion?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I believe I can. I left it out earlier because javascript also didn't seem to accept that

Copy link
Author

Choose a reason for hiding this comment

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

@josdejong I was able to make this happen as well by modifying a part of the parseObject function.

Right before it would throw an error due to not finding a symbol, string or number as an object key, if the current token is '-', it would try to parseUnary and if the resulting node is an operator node with just a constant node with a number value, then it would proceed to use the number preceeded by a '-' as the key

Copy link
Owner

Choose a reason for hiding this comment

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

I think you're idea is the same as Glen implemented for the % operator just yesterday in #3505, that indeed works out nicely.

}, /Symbol, numeric literal or string expected as object key \(char 2\)/)
})

it('should coerce numbers to string when trying to access an object expression property with matrix index', function () {
assert.strictEqual(parseAndEval('{2: 6}[2]'), 6)
assert.strictEqual(parseAndEval('{2.5: 6}[2.5]'), 6)
assert.strictEqual(parseAndEval('{5: 16}[3]'), undefined)
})

it('should accept operations in a matrix when trying to access an object expression property', function () {
assert.strictEqual(parseAndEval('{2: 6}[2 * 1]'), 6)
assert.strictEqual(parseAndEval('{31: 7 - 4}[0.2 + 0.8]'), undefined)
assert.strictEqual(parseAndEval('{4: 11 * 4}[(2 ^ 2) * 1]'), 44)
})

it('should ignore leading zeros when trying to apply numeric keys in an object expression', function () {
assert.deepStrictEqual(parseAndEval('{02: 6}'), { 2: 6 })
assert.deepStrictEqual(parseAndEval('{0070: 6}'), { 70: 6 })
assert.deepStrictEqual(parseAndEval('{0.2: 6}'), { 0.2: 6 })
assert.deepStrictEqual(parseAndEval('{0010.0501: "haha"}'), { 10.0501: 'haha' })
})

it('should ignore leading zeros in a matrix index when trying to access an object expression property', function () {
assert.strictEqual(parseAndEval('{2: 6}[02]'), 6)
assert.strictEqual(parseAndEval('{70: 1 - 6}[0070]'), -5)
assert.strictEqual(parseAndEval('{0.2: 6}[000.2]'), 6)
assert.strictEqual(parseAndEval('{10.0501: "haha"}[0010.0501]'), 'haha')
})

it('should set a nested matrix subset from an object property (1)', function () {
const scope = { obj: { foo: [1, 2, 3] } }
assert.deepStrictEqual(parseAndEval('obj.foo[2] = 6', scope), 6)
Expand Down