Cleanup ee9 DefaultServletTest #11927

joakime · 2024-06-17T17:14:14Z

A cleanup of the test setup in the ee9 DefaultServletTest.

This includes re-enabling various disabled tests.

This is needed for testing alternate Base Resource configurations for the upcoming PR to address issue #11925 which will utilize this refactor of the DefaultServletTest.

janbartel · 2024-06-18T06:21:10Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

-        defholder.setInitParameter("dirAllowed", "true");
-        defholder.setInitParameter("redirectWelcome", "false");
-        defholder.setInitParameter("gzip", "false");
+        Path docRoot = workDir.getEmptyPathDir().resolve("docroot");


These 2 lines seem to be repeated by every test, move into @BeforeEach?

No, as this logic is not consistent across the test cases, only about 70% of them use the docroot.

Also, the new testcases coming after this PR is merged (see the comment at the top of this PR) from Issue #11925 will not use this docroot as well (that PR will use many alternate docroot concepts that have been shown to cause issues)

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

janbartel · 2024-06-18T06:25:48Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

+
+        Path extraJarResources = MavenPaths.findTestResourceFile(ODD_JAR);
+        assertTrue(Files.exists(extraJarResources), "Unable to find " + ODD_JAR);
+        URL[] urls = new URL[]{extraJarResources.toUri().toURL()};


Why do we need the extra classloader? The test resource will already be on the current classpath won't it?

This jar comes from src/test/resources/odd_jar.jar and isn't "live".
These test cases are specifically handling reported behaviors with odd/unusual jars.

Since a JAR can have entries that are difficult (and in some cases impossible) on real File System (unix, mac, and windows are all included in this statement), this test case ensures that we can support these kinds of files.

I don't really understand your answer, as all things in src/test/resources are always on the test classpath, but if you want to create a special classloader for it, ok.

janbartel · 2024-06-18T06:31:33Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

@@ -471,7 +529,7 @@ public static Stream<Arguments> contextBreakoutScenarios()
            );

            // A Raw Question mark in the prefix can be interpreted as a query section
-            if (prefix.contains("?"))
+            if (prefix.contains("?") || prefix.contains(";"))


The comment above now doesn't match the code - why are we now including semicolon?

The tests using this methodSource were disabled in ee9.

In ee10, this same testcase is enabled.
The changes in HttpURI make some paths interpret differently here.

In ee10, we have ...

jetty.project/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/DefaultServletTest.java

Lines 864 to 865 in 745cba4

// A Raw Question mark in the prefix can be interpreted as a query section

if (prefix.contains("?") || prefix.contains(";"))

Basically, paths like /context/dir;/../index.html are interpreted in Jetty 12 as /context/dir/../index.html
The change in the ee10 DefaultServletTest addresses this new reality of HttpURI, so that same change is now applied to the recently enabled tests in ee9 as well.

OK well maybe we should put a javadoc comment in there to that effect?

janbartel · 2024-06-18T06:48:56Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

+        {
+            context.setBaseResource(ResourceFactory.of(context).newResource(docRoot));
+
+            ServletHolder defholder = context.addServlet(DefaultServlet.class, "/*");


I know it's this way in the original file, however the DefaultServlet is normally mapped at /, not /*

Looking back historically, this seems intentional.
This servlet isn't declared with the "default" name either.
It seems to want to replicate a end user use-case.

Keep in mind that the new test cases coming after this PR is merged will introduce even more horrible DefaultServlet usages.

This (and subsequent tests with same quirk) are from the issue marked JETTY-980 from release jetty-7.0.0.M2.

It looks like the automatic adding of DefaultServlet to / if a servlet is not present on that url-pattern was one of the behaviors that triggered the original issue.
Because users would add another DefaultServlet to /* and that extra DefaultServlet on the /* pattern was causing issues with directory listings.

OK, maybe a javadoc comment then to ensure that in future we know this is intentional for the test.

janbartel · 2024-06-18T06:49:11Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

+        {
+            context.setBaseResource(ResourceFactory.of(context).newResource(docRoot));
+
+            ServletHolder defholder = context.addServlet(DefaultServlet.class, "/*");


I know it's this way in the original file, however the DefaultServlet is normally mapped at /, not /*

Same as above, this is intentional, and historical, to match end user use cases.

janbartel · 2024-06-18T06:49:22Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

+        {
+            context.setBaseResource(ResourceFactory.of(context).newResource(docRoot));
+
+            ServletHolder defholder = context.addServlet(DefaultServlet.class, "/*");


I know it's this way in the original file, however the DefaultServlet is normally mapped at /, not /*

Same as above, this is intentional, and historical, to match end user use cases.

janbartel · 2024-06-18T06:50:14Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

+        {
+            context.setBaseResource(ResourceFactory.of(context).newResource(docRoot));
+
+            ServletHolder defholder = context.addServlet(DefaultServlet.class, "/*");


I know it's this way in the original file, however the DefaultServlet is normally mapped at /, not /*

Same as above, this is intentional, and historical, to match end user use cases.

janbartel

Does ee10 DefaultServlet need the same changes applied?

joakime · 2024-06-18T11:15:15Z

Does ee10 DefaultServlet need the same changes applied?

Possibly, but that can happen in a different PR, as the ee10 DefaultServletTest has some other differences.
I'm trying to avoid massive PRs to review.

janbartel

OK, but see comments about javadocs.

janbartel · 2024-06-19T02:35:50Z

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java

+        {
+            context.setBaseResource(ResourceFactory.of(context).newResource(docRoot));
+
+            ServletHolder defholder = context.addServlet(DefaultServlet.class, "/*");


OK, maybe a javadoc comment then to ensure that in future we know this is intentional for the test.

+ Fix various warnings + Disable ODD_JAR / extraClassPath (temporarily) + Reenable disabled test Test + Reenable disabled test testListingContextBreakout

joakime requested review from lorban and lachlan-roberts June 17, 2024 18:19

joakime self-assigned this Jun 17, 2024

joakime added the Test label Jun 17, 2024

joakime marked this pull request as ready for review June 17, 2024 18:20

joakime mentioned this pull request Jun 17, 2024

java.lang.NullPointerException: Cannot invoke "String.startsWith(String)" because "etag" is null #11925

Closed

janbartel reviewed Jun 18, 2024

View reviewed changes

jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/DefaultServletTest.java Show resolved Hide resolved

janbartel reviewed Jun 18, 2024

View reviewed changes

joakime requested a review from janbartel June 18, 2024 12:59

janbartel previously approved these changes Jun 19, 2024

View reviewed changes

Cleanup ee9 DefaultServletTest

4f72b84

+ Fix various warnings + Disable ODD_JAR / extraClassPath (temporarily) + Reenable disabled test Test + Reenable disabled test testListingContextBreakout

joakime dismissed janbartel’s stale review via 4f72b84 June 19, 2024 15:22

joakime force-pushed the fix/12.0.x/ee9-defaultservlettest-update branch from e036a44 to 4f72b84 Compare June 19, 2024 15:22

joakime merged commit 9db54aa into jetty-12.0.x Jun 19, 2024
10 checks passed

joakime deleted the fix/12.0.x/ee9-defaultservlettest-update branch June 19, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup ee9 DefaultServletTest #11927

Cleanup ee9 DefaultServletTest #11927

joakime commented Jun 17, 2024 •

edited

Loading

janbartel Jun 18, 2024

joakime Jun 18, 2024 •

edited

Loading

janbartel Jun 18, 2024

joakime Jun 18, 2024

joakime Jun 18, 2024

janbartel Jun 19, 2024

janbartel Jun 18, 2024

joakime Jun 18, 2024

janbartel Jun 19, 2024

janbartel Jun 18, 2024

joakime Jun 18, 2024

joakime Jun 18, 2024

janbartel Jun 19, 2024

janbartel Jun 18, 2024

joakime Jun 18, 2024

janbartel Jun 18, 2024

joakime Jun 18, 2024

janbartel Jun 18, 2024

joakime Jun 18, 2024

janbartel left a comment

joakime commented Jun 18, 2024

janbartel left a comment

janbartel Jun 19, 2024

	// A Raw Question mark in the prefix can be interpreted as a query section
	if (prefix.contains("?") \|\| prefix.contains(";"))

Cleanup ee9 DefaultServletTest #11927

Cleanup ee9 DefaultServletTest #11927

Conversation

joakime commented Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

joakime Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbartel left a comment

Choose a reason for hiding this comment

joakime commented Jun 18, 2024

janbartel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime commented Jun 17, 2024 •

edited

Loading

joakime Jun 18, 2024 •

edited

Loading