Re: [PR] AVRO-4229 - Update StatsServer resource handler [avro]

["martin-g (via GitHub)" <gi...@apache.org>]

martin-g commented on code in PR #3648:
URL: https://github.com/apache/avro/pull/3648#discussion_r2802572823


##########
lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StatsServer.java:
##########
@@ -42,11 +46,24 @@ public StatsServer(StatsPlugin plugin, int port) throws Exception {
     this.httpServer = new Server(port);
     this.plugin = plugin;
 
-    ServletHandler handler = new ServletHandler();
-    httpServer.setHandler(handler);
-    handler.addServletWithMapping(new ServletHolder(new StaticServlet()), "/");
+    ServletContextHandler servletContext = new ServletContextHandler(ServletContextHandler.SESSIONS);
+    servletContext.setContextPath("/");
 
-    handler.addServletWithMapping(new ServletHolder(new StatsServlet(plugin)), "/");
+    ServletHolder servletHolder = new ServletHolder(new StatsServlet(plugin));
+    servletContext.addServlet(servletHolder, "/");
+
+    ResourceHandler resourceHandler = new ResourceHandler();
+    resourceHandler.setBaseResource(Resource.newClassPathResource("/org/apache/avro/ipc/stats/static"));

Review Comment:
   ```suggestion
   Resource baseResource = Resource.newClassPathResource("/org/apache/avro/ipc/stats/static");
   if (baseResource == null) {
       throw new IllegalStateException("Static resources not found on classpath");
   }
   resourceHandler.setBaseResource(baseResource);
   ```
   since the classpath is provided by another Maven module IMO it would be good to make this check here and fail early if for some reason this package is not available



##########
lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StatsServer.java:
##########


Review Comment:
   Not caused by this PR but we could improve it.
   The ASFv2 licence should be at the top of the file.



##########
lang/java/ipc-jetty/src/main/java/org/apache/avro/ipc/jetty/StatsServer.java:
##########
@@ -42,11 +46,24 @@ public StatsServer(StatsPlugin plugin, int port) throws Exception {
     this.httpServer = new Server(port);
     this.plugin = plugin;
 
-    ServletHandler handler = new ServletHandler();
-    httpServer.setHandler(handler);
-    handler.addServletWithMapping(new ServletHolder(new StaticServlet()), "/");
+    ServletContextHandler servletContext = new ServletContextHandler(ServletContextHandler.SESSIONS);
+    servletContext.setContextPath("/");
 
-    handler.addServletWithMapping(new ServletHolder(new StatsServlet(plugin)), "/");
+    ServletHolder servletHolder = new ServletHolder(new StatsServlet(plugin));
+    servletContext.addServlet(servletHolder, "/");
+
+    ResourceHandler resourceHandler = new ResourceHandler();
+    resourceHandler.setBaseResource(Resource.newClassPathResource("/org/apache/avro/ipc/stats/static"));
+    resourceHandler.setDirectoriesListed(false); // Optional: prevent directory listing
+
+    ContextHandler staticContext = new ContextHandler();
+    staticContext.setContextPath("/static");
+    staticContext.setHandler(resourceHandler);
+
+    HandlerList handlers = new HandlerList();
+    handlers.setHandlers(new org.eclipse.jetty.server.Handler[] { staticContext, servletContext });

Review Comment:
   this can be simplified to:
   ```suggestion
       HandlerList handlers = new HandlerList(staticContext, servletContext);
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org