Log deprecation warning when using built-in JettyLauncher #9866
Conversation
This patch has two approaches to check if the JettyLauncher is being used, it checks for any configured filters/servlets (other than the defaults provided by Jetty) in order to log a warning on startup, and it checks for non-404 responses to catch other resources loaded after startup. No warning is logged for tests. Fixes gwtproject#9863
|
Created as a draft for now, to make sure the scope is as expected. Note that I did end up logging when any non-404 response is given from the server, rather than worry about static content vs dynamic Tested to confirm that tests will not log this warning, and verify that both the Hello (no servlets defined, logs on access) and Dynatable (servlets defined, logs on startup) samples do show it in the Jetty tab. |
| boolean hasNonJettyFiltersOrServlets = Stream.concat( | ||
| getServletContext().getServletRegistrations().values().stream(), | ||
| getServletContext().getFilterRegistrations().values().stream() | ||
| ).anyMatch(r -> !r.getClassName().startsWith("org.eclipse.jetty")); |
There was a problem hiding this comment.
Fwiw, this would exclude explicit registrations of servlets and filters from the Jetty packages (another instance of the default servlet, a QoS or Header filter)
It won't match configurations of security constraints or environmental entries either, as well as ServletContainerInitializers (that could store values in the servlet context, or register e.g. session listeners, without registering servlets and filters).
IIRC, people can also put a jetty-web.xml in their WEB-INF to configure the Jetty WebAppContext further, that wouldn't be detected here.
So, while it would match the majority of cases, it will miss a few edge-cases.
I wonder if we shouldn't try to match loading of WEB-INF/web.xml, META-INF/web-fragment.xml, META-INF/services/javax.servlet.ServletContextInitializer, META-INF/jetty-web.xml (and the web-jetty.xml and other variants)
And/or maybe if the WebAppClassLoaderExtension loads any class we haven't explicitly allowed? (might not detect the QosFilter or HeaderFilter, or explicit DefaultServlet, so would have to be combined with the other resource-based approach?)
There was a problem hiding this comment.
While you're correct that we would miss these edge cases on startup, I'll bet that we're still missing cases - that's why any non-404 response (including opening a websocket unless I'm very much mistaken) will also trigger the warning to log (though not right at startup, but in limited testing they appear before the ncsa-style log entries). Does that seem like a sufficient mechanism for catch-all, or do you think we need to get, traverse all of the supported configuration types as well?
There was a problem hiding this comment.
I was thinking on using the classloader's resource loading for those files I listed. Do you think that would work?
Anyway, better merge this sooner than later so feel free to merge as-is.
|
In lieu of other discussion, I'm marking this as "ready for review". |
Migrated shared classes to their own types, and deprecated JettyLauncher with the expectation of removing it in some future release.
|
Added a simple server implementation that can serve static resources (and will be easier to maintain, won't care about javax vs servlet, jetty upgrades, etc) that can be turned on via |
Good idea to provide a static content launcher for solutions that don't need servlets etc. |
|
Sounds great - are you interested in starting such a project? Feel free to tag me in at least as a reviewer of work - I'm sure the gwt-eclipse-plugin project would welcome making such a feature available via some simple configuration as well? Making the launcher a separate project would enable different implementations per version of jetty/tomcat/etc as well, giving the user the option of when to upgrade, rather than GWT releases forcing all teams to update at once. |
|
This seems to be related to #9863 where an alternative I wouldn't know where to start but I am guessing that this could be the way forward for GWT > 2.10.0 with Jetty launcher sadly removed, for good reasons after all which I understand. I certainly agree with making this a separate GitHub project for flexibility. |
|
With Frank making changes to gwt-site, I'm thinking this is ready to merge, with a later (in 2.12+) follow-up to replace samples and remove the ant webAppCreator. I'll file one more PR to deprecate webAppCreator in 2.11, pushing users towards the maven archetypes (and we can document Gradle samples once they exist - any volunteers?). Any objections? |
This patch has two approaches to check if the JettyLauncher is being used, it checks for any configured filters/servlets (other than the defaults provided by Jetty) in order to log a warning on startup, and it checks for non-404 responses to catch other resources loaded after startup.
No warning is logged for tests.
Fixes #9863