Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RegexPathSpec documentation and MatchedPath improvements #8163

Merged
merged 7 commits into from
Jun 16, 2022

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Jun 13, 2022

Added javadoc, and improved RegexMatchedPath behavior with more unit tests.

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from gregw June 13, 2022 20:38
@joakime joakime added this to In progress in Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 via automation Jun 13, 2022
@joakime joakime self-assigned this Jun 13, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I think you have well captured the current behaviour, but I think we should consider changing it a little as per my comment.

Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from In progress to Review in progress Jun 13, 2022
+ Allow override of split index determination

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I think the logic is getting a bit complex and a bit burried. I've made a suggestion for how the description should be, but I think the following algorithm should be used as it is a bit more efficient with less name lookups:

switch(matcher.groupCount())
{
    case 0: match=path; info=null; 
    break;

    case 1:
      if ("name".equals(group(1).name)
          match=group; info=path.substring(group.end);
      else
          match=path.substring(0,group.start); info=group; 
      break;

    default:
      if (group("name").exists)
      {   
          match=group("name");
              info=group("info");
          else
              info=path.substring(group("name").end);
      }
      else if (group("info").exists)
          match=path.substring(0,group.start); info=group; break;
      else
          match=path; info=null; 
    break;
}

+ Cleanup testcase for MatchedPath
+ Test cases are now failing, need to rework algorithm.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor Author

joakime commented Jun 15, 2022

@gregw I implemented your algorithm, but it makes quite the mess. (lots of test failures)
Take a look at the test case examples.

https://github.com/eclipse/jetty.project/blob/52de789437b5415ad7a1c594207e2d6d4c60fb05/jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/RegexPathSpecTest.java#L90-L126

The test argument order is ...
String regex, String input, String expectedPathMatch, String expectedPathInfo

Do you think these are sane expectations?
If so, then we need to fix the algorithm, or reintroduce the check on PREFIX_GLOB.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
gregw
gregw previously approved these changes Jun 15, 2022
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I think this is good enough for now, but with TODOs for future optimizations.

{
if (idxNameStart >= 0)
// we know we are splitting
int idxNameEnd = endOf(matcher, "name");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ultimately this lookup makes me itch! If they are not using the named group, then an exception is created and ignored for every lookup attempt.

For now, can we add a TODO that when we scan the pattern, we should look for the presence of "" and only do this lookup if we have seen it in the pattern.

Comment on lines +436 to +437
String gName = valueOf(matcher, "name");
String gInfo = valueOf(matcher, "info");
Copy link
Contributor

Choose a reason for hiding this comment

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

Two exceptions may be created and ignored here. So again, I think we need a TODO to optimise that away in future.

Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 15, 2022
@joakime
Copy link
Contributor Author

joakime commented Jun 16, 2022

Once the javadoc issues on PR #8173 are green and merged, this should build green, then I can merge this one.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Review in progress Jun 16, 2022
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Review in progress to Reviewer approved Jun 16, 2022
@joakime joakime merged commit 1b4f941 into jetty-10.0.x Jun 16, 2022
Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from Reviewer approved to Done Jun 16, 2022
@joakime joakime deleted the fix/jetty-10-regexpathspec-doc branch June 16, 2022 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants