Issue #10661 Allow jetty api to override servlets and mappings from webdefault #10668

janbartel · 2023-10-04T22:08:14Z

…ebdefault

joakime

One test case assertion seems incomplete, and then some odd indenting.
Nothing that would hold up this PR IMO, but it should be addressed.

...ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessorTest.java

jetty-ee10/jetty-ee10-webapp/src/test/resources/web-redefine-mapping-fail.xml

jetty-ee10/jetty-ee10-webapp/src/test/resources/web-redefine-mapping.xml

...y-ee9-webapp/src/test/java/org/eclipse/jetty/ee9/webapp/StandardDescriptorProcessorTest.java

jetty-ee9/jetty-ee9-webapp/src/test/resources/web-redefine-mapping-fail.xml

jetty-ee9/jetty-ee9-webapp/src/test/resources/web-redefine-mapping.xml

…10661-jetty-api-webdefaults

joakime · 2023-10-06T02:11:28Z

...ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessorTest.java

+                assertEquals(null, mapping);
+                mapping = m;
+            }
+        }


So you iterate through all of the mappings, what if there are 2 mappings both with path spec /? (I know this shouldn't be possible, but you found we accidentally did just that before this PR, and we should ensure that we don't have that regression again)

Line 147 is doing exactly this.

joakime

Check for duplicate mapping

...y-ee9-webapp/src/test/java/org/eclipse/jetty/ee9/webapp/StandardDescriptorProcessorTest.java

...ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessorTest.java

...tty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessor.java

gregw · 2023-10-08T22:03:22Z

...tty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessor.java

@@ -1054,34 +1063,41 @@ public ServletMapping addServletMapping(String servletName, XmlParser.Node node,
                    {
                        //The same path has been mapped multiple times, either to a different servlet or the same servlet.
                        //If its a different servlet, this is only valid to do if the old mapping was from a default descriptor.


If it is valid to change a mapping from a default descriptor... and we allow embedded code to be used instead of a default descriptor, then why isn't it valid to do if the old mapping was from embedded?
I.e. If we define a servlet in embedded it is seen as a default, but a mapping appears to be handled differently?

Not sure exactly what should be done here... but either way probably need to comment to exact reasoning.

If we allow descriptors to override embedded, then a user can never change a mapping that came from the default descriptor. The principle of least surprise says to me that if I define a servlet and mapping in embedded, I would expect those to be honoured, and not obscured by something from the defaults descriptor, which is mostly hidden from users buried inside jetty-webapp jar.

@agree I think we agree on the action, but disagree on the description. If either a servlet or a mapping is defined by embedded code, then it should not be overridden by the default descriptor, but could be overridden by a normal descriptor.

Said another way, the default for either Servlet or Mapping comes from either embedded or if not set there, then the webdefaults xml. Either default can be overridden by a normal descriptor.
So what I'd like to see is the same logic here as is done for a servlet. If the mapping is already defined and we are the default descriptor and the mapping came from embedded, then skip the current mapping.

Does the latest commit make this any clearer?

…10661-jetty-api-webdefaults

…e10/webapp/StandardDescriptorProcessor.java Co-authored-by: Greg Wilkins <gregw@webtide.com>

…10661-jetty-api-webdefaults

gregw

Sorry, it is better... but I think it just reveals more questions about what we are doing in the first place. I don't understand why servlets are so simple and this is so complex.

I think we are going to need to collaborate on a solution.

...tty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessor.java

janbartel

I'm happy with your latest changes @gregw so if you can give this a +1 I'll get it committed.

…10661-jetty-api-webdefaults

Issue #10661 Allow jetty api to override servlets and mappings from w…

8c50e0b

…ebdefault

janbartel self-assigned this Oct 4, 2023

janbartel requested a review from gregw October 4, 2023 22:08

janbartel added 3 commits October 5, 2023 10:17

Add reads for StacklessLogging; remove extraneous println

841115c

Fix cdi tests for clashing servlet pathspec

1017b3c

Apply fix to ee9 and ee8

46c556e

joakime reviewed Oct 5, 2023

View reviewed changes

Changes post review

f5aac57

janbartel requested a review from joakime October 6, 2023 00:01

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…

08aa1c5

…10661-jetty-api-webdefaults

joakime reviewed Oct 6, 2023

View reviewed changes

joakime requested changes Oct 6, 2023

View reviewed changes

...y-ee9-webapp/src/test/java/org/eclipse/jetty/ee9/webapp/StandardDescriptorProcessorTest.java Show resolved Hide resolved

...ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/StandardDescriptorProcessorTest.java Show resolved Hide resolved

janbartel requested a review from joakime October 6, 2023 05:24

gregw requested changes Oct 8, 2023

View reviewed changes

janbartel and others added 6 commits October 9, 2023 10:13

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…

57238d7

…10661-jetty-api-webdefaults

Change after review.

8a080d4

Update jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/e…

6a55b44

…e10/webapp/StandardDescriptorProcessor.java Co-authored-by: Greg Wilkins <gregw@webtide.com>

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…

71418f1

…10661-jetty-api-webdefaults

Changes post review

5d69e3e

Add more test

c5da5ee

janbartel requested a review from gregw October 10, 2023 04:05

gregw requested changes Oct 10, 2023

View reviewed changes

updates from review

789e454

janbartel commented Oct 11, 2023

View reviewed changes

gregw previously approved these changes Oct 11, 2023

View reviewed changes

joakime previously approved these changes Oct 16, 2023

View reviewed changes

joakime linked an issue Oct 18, 2023 that may be closed by this pull request

Ensure jetty api servlets/filters take precedence over webdefault.xml declarations. #10661

Closed

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12.0.x-…

965ebdd

…10661-jetty-api-webdefaults

janbartel dismissed stale reviews from joakime and gregw via 965ebdd October 19, 2023 03:24

janbartel requested a review from gregw October 19, 2023 04:21

gregw approved these changes Oct 19, 2023

View reviewed changes

janbartel merged commit 14a5ba3 into jetty-12.0.x Oct 19, 2023
2 checks passed

janbartel deleted the jetty-12.0.x-10661-jetty-api-webdefaults branch October 19, 2023 06:35

janbartel mentioned this pull request Oct 19, 2023

Ensure jetty api servlets/filters take precedence over webdefault.xml declarations. #10661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #10661 Allow jetty api to override servlets and mappings from webdefault #10668

Issue #10661 Allow jetty api to override servlets and mappings from webdefault #10668

janbartel commented Oct 4, 2023

joakime left a comment

joakime Oct 6, 2023

janbartel Oct 6, 2023

joakime left a comment

gregw Oct 8, 2023

janbartel Oct 8, 2023

gregw Oct 9, 2023

janbartel Oct 10, 2023

gregw left a comment

janbartel left a comment

Issue #10661 Allow jetty api to override servlets and mappings from webdefault #10668

Issue #10661 Allow jetty api to override servlets and mappings from webdefault #10668

Conversation

janbartel commented Oct 4, 2023

joakime left a comment

Choose a reason for hiding this comment

joakime Oct 6, 2023

Choose a reason for hiding this comment

janbartel Oct 6, 2023

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

gregw Oct 8, 2023

Choose a reason for hiding this comment

janbartel Oct 8, 2023

Choose a reason for hiding this comment

gregw Oct 9, 2023

Choose a reason for hiding this comment

janbartel Oct 10, 2023

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

janbartel left a comment

Choose a reason for hiding this comment