Skip to content

matcher: fix quantifiers that follow a {n,m} repeat - #1306

Open
RaphaelFakhri wants to merge 1 commit into
Netflix:mainfrom
RaphaelFakhri:fix-optional-group-repeat
Open

RaphaelFakhri wants to merge 1 commit into
Netflix:mainfrom
RaphaelFakhri:fix-optional-group-repeat

Conversation

@RaphaelFakhri

Copy link
Copy Markdown
Contributor

Fixes #1305

Parser.meta() treats any ? that follows a RepeatMatcher as the reluctant modifier of the repetition and returns the repetition unchanged. A group whose body compiles to a single RepeatMatcher, such as (\d{2}), takes the same path, so (\d{2})? loses its ? and the group becomes required. ^v(\d{2})?$ does not match v.

A + after {n,m} is not consumed either, so the possessive a{2}+ is parsed as (a{2})+, and ^a{2}+$ matches aaaa.

Changes

  • In Parser.repeat(), consume an optional ? or + modifier directly after the closing }. RepeatMatcher already matches greedily without backtracking, so both modifiers keep the current matching behavior for the repetition itself.
  • Remove the RepeatMatcher special case from meta(), so a ? after a group always makes the group optional.
  • Add repeatInOptionalGroup and repeatModifiers to AbstractPatternMatcherTest. They compare the results with java.util.regex.Pattern for all of the matcher test variants.

Testing

./gradlew :spectator-api:test --tests 'com.netflix.spectator.impl.matcher.*'
  • Without the fix: 629 tests, 10 failures (both new tests in PatternMatcherTest, CaseInsensitivePatternMatcherTest, ExpandOrPatternMatcherTest, Re2PatternMatcherTest, and Re2CaseInsensitivePatternMatcherTest), for example ^v(\d{2})?$ should match v and ^a{2}+$ shouldn't match aaaa.
  • With the fix: 629 tests, 0 failures.
  • ./gradlew :spectator-api:build (checkstyle, PMD, SpotBugs, license, tests on JDK 8, 17, and 25), ./gradlew license, and ./gradlew :spectator-reg-atlas:test pass.

Parser.meta() treated any '?' after a RepeatMatcher as the reluctant
modifier of the repetition. A group whose body ends up as a single
RepeatMatcher, such as (\d{2})?, lost its '?' and became required, so
^v(\d{2})?$ did not match "v". A possessive modifier such as a{2}+ was
parsed as (a{2})+ instead, so ^a{2}+$ matched "aaaa".

Consume the optional '?' or '+' modifier when parsing the {n,m} repeat
so later quantifiers apply to the preceding term as usual.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PatternMatcher drops ? after a group that contains only a {n,m} repeat

1 participant