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

Cleanup of TypeUtil and ContextHandler stop/start #8998

Merged
merged 5 commits into from
Dec 6, 2022

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Dec 5, 2022

Hopefully non controversial changes from monster PR:

  • TypeUtil class shortname used more often and includes trailing digits
  • Fixed direct stopping/starting of a nested ContextHandler
  • Fixed null path handling in nested context
  • more tests for all of the above

 + TypeUtil class shortname used more often and includes trailing digits
 + Fixed direct stopping/starting of a nested ContextHandler
 + Fixed null path handling in nested context
 + more tests for all of the above
@gregw
Copy link
Contributor Author

gregw commented Dec 5, 2022

@joakime @lachlan-roberts the CI failure is a java-19 flake. Can you please review rather than wait for a green build.

@gregw gregw changed the title Extracted some non controversial cleanups from another mega PR: Cleanup of TypeUtil and ContextHandler stop/start Dec 5, 2022
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@@ -490,7 +490,7 @@ public void testBaseResourceAbsolutePath() throws Exception
// On Unix / Linux this should have no issue.
// On Windows with fully qualified paths such as "E:\mybase\webapps\dump.war" the
// resolution of the Resource can trigger various URI issues with the "E:" portion of the provided String.
context.setResourceBase(warPath.toString());
context.setBaseResourceAsString(warPath.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI again, I think this is also part of 2 other PRs.

@gregw gregw merged commit e682f73 into jetty-12.0.x Dec 6, 2022
@gregw gregw deleted the jetty-12.0.x-cleanups branch December 6, 2022 04:43
@@ -141,6 +141,44 @@ public void testSimpleGET() throws Exception
assertThat(BufferUtil.toString(stream.getResponseContent()), equalTo(helloHandler.getMessage()));
}

@Test
public void testNullPath() throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we need processMovedPermanently and processByContextHandler both in ContextHandler one sends a 301 and other sends 302 response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do. I've removed processByContextHandler in #8978

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.

4 participants