disallow empty attribute names in query language #108
Conversation
Currently, an attribute name can be any sequence of identifiers and dots. For example, the parser will except a selector like:
```
[...hi..hello]
```
This can lead to issues in the getPath function (esquery.js#L35) when the full name is split around the dots and used to index into a node.
This pull request modifies the attribute name rule to only accept identifiers separated by exactly one dot.
The resulting array from the rule is flattened using the array reduce function. I noticed a comment further down in the grammar that seemed to indicate that the array flat() function is not supported. If it is supported, the transformation can be simplified to
```js
return [a, ...as.reduce((acc, val) => acc.concat(val), [])].join('');
```
Co-authored-by: Michael Ficarra <github@michael.ficarra.me>
|
While there's nothing theoretically wrong with accepting the empty string as a property name, I don't really see a practical need for it, so I'm okay with this. You're going to need to drop that test that you mention for CI to pass. |
michaelficarra
changed the title
|
Thanks for the contribution, @MarkKoester! |
|
As far as the change, I'd personally worry that people might want, e.g., to make ESLint rules which checked for empty string values or such. But I admittedly don't have a personal need for it. As far as the coverage, FWIW, IIRC, the coverage I was speaking of in that test was in regard to the JS parser file, which one can meaningfully get coverage of in using the fork in this PR (which adds ignore statements to PEG.js boilerplate or else branches of generally less interest). |
|
Yeah I actually didn't know you were allowed to use the empty string as a key but I suppose that would actually be a valid use. I still think this is a good change to make since there are already a lot of valid JavaScript object keys that technically wouldn't parse, for example ">". Another minor point is that I think if you actually wanted to support using the empty string as an object key it should still parse as an identifierName with the value of an empty string. It was only technically supported before because two dots in a row happened to output the same thing as an identifier with an empty value. The fact that it was allowed in the first place was sort of unintentional. |
Currently, an attribute name can be any sequence of identifiers and dots. For example, the parser will except a selector like:
The getPath function will probably end up returning undefined in any case when it indexes an object using an empty string.
This pull request modifies the attribute name rule to only accept identifiers separated by exactly one dot.
The resulting array from the rule is flattened using the array reduce function. I noticed a comment further down in the grammar that seemed to indicate that the array flat() function is not supported. If it is supported, the transformation can be simplified to
It seems like there's a unit test that expects the parser to accept attribute names with a leading dot. I'm not sure what the purpose of this is since the getPath function will always return undefined in this case, but if this is the intended behavior then the grammar rule can easily be modified to accept it.