Copilot commented on code in PR #10706:
URL: https://github.com/apache/gravitino/pull/10706#discussion_r3048904402
##########
server/src/main/java/org/apache/gravitino/server/GravitinoServer.java:
##########
@@ -175,9 +175,6 @@ protected void configure() {
server.addCustomFilters(API_ANY_PATH);
server.addFilter(new VersioningFilter(), API_ANY_PATH);
server.addSystemFilters(API_ANY_PATH);
Review Comment:
Web UI routing/SPA behavior likely breaks when UI is enabled because
WebUIFilter is no longer registered. web/web/WEB-INF/web.xml doesn’t define
this filter, and WebUIFilter is responsible for forwarding "/" and "/ui/*"
routes to the correct static HTML (e.g., /ui/index.html and /ui/<route>.html).
Consider adding WebUIFilter back conditionally when Configs.SERVER_UI_ENABLED
is true (using the same flag passed into JettyServer.initialize).
##########
server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java:
##########
@@ -99,6 +100,25 @@ public void testStartAndStop() throws Exception {
gravitinoServer.stop();
}
+ @Test
+ public void testStartAndStopWithWebUiDisabled() throws Exception {
+ ServerConfig config = new ServerConfig();
+ config.loadFromMap(
+ ImmutableMap.of(
+ GravitinoServer.WEBSERVER_CONF_PREFIX +
JettyServerConfig.WEBSERVER_HTTP_PORT.getKey(),
+ String.valueOf(RESTUtils.findAvailablePort(5000, 6000)),
+ Configs.SERVER_UI_ENABLED.getKey(),
+ "false",
+ AuxiliaryServiceManager.GRAVITINO_AUX_SERVICE_PREFIX
+ + AuxiliaryServiceManager.AUX_SERVICE_NAMES,
+ ""),
+ t -> true);
+ GravitinoServer localServer = new GravitinoServer(config,
GravitinoEnv.getInstance());
+ localServer.initialize();
+ localServer.start();
+ localServer.stop();
+ }
Review Comment:
This new test likely doesn’t validate the intended behavior because unit
tests run with GRAVITINO_TEST set (see server/build.gradle.kts), and
JettyServer already tolerates missing WARs in that mode even when UI is
enabled. As a result, the test would still pass if Configs.SERVER_UI_ENABLED
were ignored. Consider strengthening it by asserting that UI is actually
disabled (e.g., verify JettyServer.initialize is called with
shouldEnableUI=false, or assert the server uses the basic ServletContextHandler
instead of WebAppContext, or run a forked test without GRAVITINO_TEST).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
