[JsonPath] Improve compliance to the RFC test suite #60699
Conversation
ba52060 to
9ceeb5f
Compare
9ceeb5f to
9e73c21
Compare
|
Thank you @stof, I didn't know that denial of service was actually a thing with regex. I updated accordingly. |
|
@alexandre-daubois this can be a thing when you allow user input for the string being matched by the regex (which could totally happen in this component). Backtracking engines (like PCRE) have an exponential complexity based on the length of the input when attempting to match an affected regex (and failing to match it, as this is the worse case of backtracking). This is commonly reported in the npm ecosystem (also because JS does not support possessive quantifiers in its Regexp syntax, and so cannot apply the easy fix to prevent them in many cases, making the issue more common) |
9e73c21 to
2c412e1
Compare
2c412e1 to
5beab82
Compare
|
Applied your suggestions, it makes the code a bit simpler. Thanks! |
5beab82 to
ab0853a
Compare
ab0853a to
9453d33
Compare
|
I reworked the whole PR to keep pushing forward the compliance test suite. This PR removes 265 skips, so that's pretty nice. Status: Needs Review |
d71e0e5 to
84193ab
Compare
84193ab to
03f1b10
Compare
| if (str_starts_with($expr, '(') && str_ends_with($expr, ')')) { | ||
| $depth = 0; | ||
| $isWrapped = true; | ||
| for ($i = 0; $i < strlen($expr); $i++) { | ||
| if ($expr[$i] === '(') { | ||
| $depth++; | ||
| } elseif ($expr[$i] === ')') { | ||
| $depth--; | ||
| if ($depth === 0 && $i < strlen($expr) - 1) { | ||
| $isWrapped = false; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
/cc @fabpot we have an issue with fabbot:
| if (str_starts_with($expr, '(') && str_ends_with($expr, ')')) { | |
| $depth = 0; | |
| $isWrapped = true; | |
| for ($i = 0; $i < strlen($expr); $i++) { | |
| if ($expr[$i] === '(') { | |
| $depth++; | |
| } elseif ($expr[$i] === ')') { | |
| $depth--; | |
| if ($depth === 0 && $i < strlen($expr) - 1) { | |
| $isWrapped = false; | |
| break; | |
| } | |
| } | |
| } | |
| if (str_starts_with($expr, '(') && str_ends_with($expr, ')')) { | |
| $depth = 0; | |
| $isWrapped = true; | |
| for ($i = 0; $i < \strlen($expr); ++$i) { | |
| if ('(' === $expr[$i]) { | |
| ++$depth; | |
| } elseif (')' === $expr[$i]) { | |
| --$depth; | |
| if (0 === $depth && $i < \strlen($expr) - 1) { | |
| $isWrapped = false; | |
| break; | |
| } | |
| } | |
| } |
| } else { | ||
| $pathPart = substr($arg, 1); | ||
| if (str_starts_with($pathPart, '[')) { |
There was a problem hiding this comment.
| } else { | |
| $pathPart = substr($arg, 1); | |
| if (str_starts_with($pathPart, '[')) { | |
| } elseif (str_starts_with($pathPart = substr($arg, 1), '[')) { |
| $args = $this->parseCommaSeparatedValues($args); | ||
| foreach ($args as $arg) { | ||
| $arg = trim($arg); | ||
| if (str_starts_with($arg, '@')) { // special handling for @ to track nodelist size |
There was a problem hiding this comment.
this will reduce the nesting level:
| if (str_starts_with($arg, '@')) { // special handling for @ to track nodelist size | |
| if (str_starts_with($arg, '$')) { // special handling for absolute paths | |
| }elseif (!str_starts_with($arg, '@')) { | |
| //[...] |
| continue; | ||
| } | ||
|
|
||
| if ($char === '\\') { |
There was a problem hiding this comment.
yoda style everywhere
doesn't running php-cs-fixer locally catch those?
|
|
||
| if (preg_match('/\b(match|search)\s*\(([^)]*)\)/', $filterExpr, $matches)) { | ||
| $args = trim($matches[2]); | ||
| if (empty($args)) { |
| if (preg_match('/\b(length|count|value)\s*\(([^)]*)\)/', $filterExpr, $matches)) { | ||
| $functionName = $matches[1]; | ||
| $args = trim($matches[2]); | ||
| if (empty($args)) { |
There was a problem hiding this comment.
please remove all usages of empty from the PR
This PR is a big step forward on making the component RFC compliant. Many more tests are now green as you can see in the
JsonPathComplianceTestSuiteTest. Mainly, it's about dealing with whitespaces, better expressions parsing and supporting!and bare literals liketrue,falseandnull.