Optimize the regex parser for InvalidResponse #1034
Conversation
Rather than using a backtracking pattern which could have performance impact, switch to using explict character ranges which work for json path seperated with periods. Mitigates sonar lint rule java:S5852 [1] - [1] https://rules.sonarsource.com/java/RSPEC-5852/?search=Using%20slow%20regular%20expressions%20is%20security-sensitive Signed-off-by: Peter Nied <[email protected]>
peternied
requested review from
AndreKurait,
chelma,
gregschohn,
lewijacn,
mikaylathompson and
sumobrian
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1034 +/- ##
============================================
- Coverage 80.55% 80.55% -0.01%
Complexity 2735 2735
============================================
Files 365 365
Lines 13611 13611
Branches 941 941
============================================
- Hits 10965 10964 -1
Misses 2068 2068
- Partials 578 579 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| @Slf4j | |||
| public class InvalidResponse extends RfsException { | |||
| private static final Pattern UNKNOWN_SETTING = Pattern.compile("unknown setting \\[(.+?)\\].+"); | |||
| private static final Pattern UNKNOWN_SETTING = Pattern.compile("unknown setting \\[([a-zA-Z0-9_.-]+)\\].+"); | |||
There was a problem hiding this comment.
Would the issue also be resolved by changing .+? -> .+
There was a problem hiding this comment.
Yes, but that expression wouldn't be correct since it could match the closing bracket or a host of invalid characters that opensearch would never use in a setting name.
- ✔️
unknown setting [foo.bar] other stuff->foo.bar - ❌
unknown setting [foo.bar] other stuff ]->foo.bar] other stuff
There was a problem hiding this comment.
I see.. the issue is that ] shouldn't be a matching character in the inner match.
"unknown setting \\[([^\\]\n]+?)\\].+" this would be the most similar to the original logic that doesn't have the bug
This change seems safe to set the characters inside the [] to those in this PR
|
Needed to retry the gradle tests, created Flaky Test: WorkCoordinatorTest.testAcquireLeaseForQuery [1] |
Description
Rather than using a backtracking pattern which could have performance impact, switch to using explict character ranges which work for json path seperated with periods.
Mitigates sonar lint rule java:S5852 [1]
Check List
New functionality includes testingNew functionality has been documentedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.