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

Jetty 12.0.x resource Servlet #11933

Merged
merged 21 commits into from
Jul 9, 2024
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jun 20, 2024

Fix #10738
This is the start of a split of the DefaultServlet into a ResourceServlet.

@gregw gregw requested review from janbartel and lorban June 20, 2024 02:49
@gregw
Copy link
Contributor Author

gregw commented Jun 20, 2024

This could be 12.0 ... or perhaps best to be 12.1

@gregw gregw marked this pull request as ready for review June 23, 2024 22:08
@gregw
Copy link
Contributor Author

gregw commented Jun 24, 2024

@lorban thoughts?

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

It's a good first step that IMHO could be merged in as-is, but it needs changes to the javadoc of ResourceServlet and probably some changes to the doc too.

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.

Seems like the DefaultServlet is now pretty much the same as the ResourceServlet? What are the actual differences?

@gregw
Copy link
Contributor Author

gregw commented Jun 24, 2024

Seems like the DefaultServlet is now pretty much the same as the ResourceServlet? What are the actual differences?

They are substantially similar, so the change is mostly in perception and future flexibility. But there are real changes in this PR:

  • In ResourceServlet the String getEncodedPathInContext(HttpServletRequest req, String includedServletPath) method is replaced with String getEncodedPathInContext(HttpServletRequest request, boolean included). This is not only more efficient, but it allows the included path to be correctly calculated by the implementation.
  • The actual implementation of String getEncodedPathInContext(HttpServletRequest request, boolean included) is more comprehensive and embraces the HttpServletMapping, rather than the half-way isDefaultMapping concept, to better determine the correct path in most circumstances
  • The pathInfoOnly configuration is re-introduced as that can have meaning for non default mappings.
  • The DefaultServlet now warns if it is mapped anywhere but the default pathspec. Once this has moved our users off using DefaultServlet, we can remove that check for a simpler/faster path calculation... or keep it to ensure that the actual default servlet has a specific sane configuration.
  • The DefaultServlet can still be configured with context init parameters, whilst the ResourceServlet cannot.

@gregw gregw requested review from lorban and janbartel June 24, 2024 22:30
@lorban lorban linked an issue Jun 26, 2024 that may be closed by this pull request
@gregw
Copy link
Contributor Author

gregw commented Jun 27, 2024

@joakime @lorban I added back the DefaultServletTest as it was, then added a bunch of parameterized tests for ResourceServletTest

@gregw gregw requested a review from janbartel June 28, 2024 07:47
lorban
lorban previously approved these changes Jul 2, 2024
@gregw
Copy link
Contributor Author

gregw commented Jul 3, 2024

@lorban slight javadoc change, so please review.
@janbartel nudge.

janbartel
janbartel previously approved these changes Jul 4, 2024
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.

+1 subject to fixing the ci build failure.

@gregw
Copy link
Contributor Author

gregw commented Jul 5, 2024

I think the CI failures are flakes (we gotta get on top of that!)

janbartel
janbartel previously approved these changes Jul 5, 2024
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

Javadoc could be improved a bit, and we need to decide what to do about that jetty-asciidoctor-extensions module.

Otherwise LGTM.

@@ -14,5 +14,6 @@

<modules>
<module>jetty</module>
<module>jetty-documentation</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also the jetty-asciidoctor-extensions module that's not activated and stuck at version 12.0.10-SNAPSHOT. Shouldn't it be enabled too?

import org.slf4j.LoggerFactory;

/**
* <p>A Servlet that handles static resources.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the javadoc say something to users willing to configure a ResourceServlet instance at /? Should we warn when we detect that?

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'm not sure anything needs to be said. They can definitely deploy a ResourceServlet at '/' and it will work fine. They just miss out on any extras or efficiencies we may put into DefaultServlet.

janbartel
janbartel previously approved these changes Jul 7, 2024
@gregw
Copy link
Contributor Author

gregw commented Jul 8, 2024

@sbordet This PR is now also trying to undo the changes introduced in #11737 that meant we no longer compiled the code used in documentation. I think I've got it right now (the poms had already stagnated), but can you double check it please?

@gregw gregw merged commit 46204a8 into jetty-12.0.x Jul 9, 2024
10 checks passed
@gregw gregw deleted the jetty-12.0.x-ResourceServlet branch July 9, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Trim down EE10 DefaultServlet Introduce EE10 ResourceServlet
4 participants