jetty.project icon indicating copy to clipboard operation
jetty.project copied to clipboard

Jetty 12.0.x resource Servlet

Open gregw opened this issue 1 year ago • 4 comments

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

gregw avatar Jun 20 '24 02:06 gregw

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

gregw avatar Jun 20 '24 02:06 gregw

@lorban thoughts?

gregw avatar Jun 24 '24 06:06 gregw

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 avatar Jun 24 '24 22:06 gregw

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

gregw avatar Jun 27 '24 08:06 gregw

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

gregw avatar Jul 03 '24 00:07 gregw

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

gregw avatar Jul 05 '24 00:07 gregw

@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 avatar Jul 08 '24 02:07 gregw