In BZ-54387 [1] a change was made to reject "Duplicate Mapping": > Duplicate mapping. As per clarification from the Servlet EG, deployment should fail. Which makes perfect sense when the duplicate mapping points to a different Servlet, but not so much when it points to the same Servlet. I have seen this break deployments even in production, when a dev did not expect this seemingly innocuous duplicate entry to break anything. I would like to propose a minor change where we throw an IllegalArgumentException [2] only if the duplicate entry is mapped to a different Servlet name. If there's no objection then I'd be happy to patch it. [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=54387 [2] https://github.com/apache/tomcat/blob/d106b2b0305e443261704ee44fa0ce8f696d1059/java/org/apache/tomcat/util/descriptor/web/WebXml.java#L346
Why duplicates should be allowed ? This is definitely a developer mistake. And it is easy to fix - just remove the second entry. How such kind of error could go to Production? Does that mean the developer never started the application locally or at Test/Staging environment ?
I'm leaning towards WONTFIX for this one. Given the spec calls this out explicitly as not allowed then I think there needs to be a very good reason to make an exception and at the moment I don't see one.
(In reply to Mark Thomas from comment #2) > I'm leaning towards WONTFIX for this one. > > Given the spec calls this out explicitly as not allowed then I think there > needs to be a very good reason to make an exception and at the moment I > don't see one. But the spec is explicit about duplicate mapping to a different servlet, right? That is not the case here as we're discussing the same servlet. In pseudocode, we're talking about the following: name = "JSP"; name = "JSP"; Is it great and efficient? No. Should it prevent the servlet from launching? IMO, no. (In reply to mgrigorov from comment #1) > Why duplicates should be allowed ? > This is definitely a developer mistake. And it is easy to fix - just remove > the second entry. Agreed on both points, but since this was not expected it took longer to identify the error and correct it. > How such kind of error could go to Production? Does that mean the developer > never started the application locally or at Test/Staging environment ? Ironically all of the initial configurations were tested in Local/Test/Staging environment, but then an urgent issue required adding more mappings and while doing so in "panic" mode, a duplicate entry was added via a copy/paste error which broke the deployment altogether, leading to down time.
The text from the spec is: <quote> If the effective web.xml (after merging information from fragments and annotations) contains any url-patterns that are mapped to multiple servlets then the deployment must fail. </quote> I've traced this back to the original EG discussion and it was only considering one URL pattern mapping to multiple Servlets. The case of duplicate mappings (same URL, same servlet) was not discussed. While I accept that the spec language is not as explicit as I thought, I remain of the view that this should be a WONTFIX. If this situation arises, clearly an error has occurred. The proposed solution assumes one particular error and "auto-corrects" based on that assumption. However, the assumption may not be valid. I think it is better to fail the deployment and require the error to be fixed rather than making assumptions about how it should be fixed.
What about specifying the (same) mapping in both @Annotations and also in WEB-INF/web.xml? If that causes an error, it would be *very* frustrating. I wouldn't consider this a bug, but rather two separate ways of specifying the same thing.
(In reply to Christopher Schultz from comment #5) > What about specifying the (same) mapping in both @Annotations and also in > WEB-INF/web.xml? If that causes an error, it would be *very* frustrating. I > wouldn't consider this a bug, but rather two separate ways of specifying the > same thing. XML config overrides the annotations.
(In reply to mgrigorov from comment #6) > (In reply to Christopher Schultz from comment #5) > > What about specifying the (same) mapping in both @Annotations and also in > > WEB-INF/web.xml? If that causes an error, it would be *very* frustrating. I > > wouldn't consider this a bug, but rather two separate ways of specifying the > > same thing. > > XML config overrides the annotations. Aha, so in that case one "replaces" the other, instead of that being a "duplicate" mapping? Then I'm okay either way on this one.
Moving to enhancement.
(In reply to Mark Thomas from comment #4) > <snip/> > > I've traced this back to the original EG discussion and it was only > considering one URL pattern mapping to multiple Servlets. The case of > duplicate mappings (same URL, same servlet) was not discussed. > > While I accept that the spec language is not as explicit as I thought, I > remain of the view that this should be a WONTFIX. If this situation arises, > clearly an error has occurred. The proposed solution assumes one particular > error and "auto-corrects" based on that assumption. However, the assumption > may not be valid. I think it is better to fail the deployment and require > the error to be fixed rather than making assumptions about how it should be > fixed. The assumption may or may not be valid, but I still think that it's harsh to fail the deployment over a no-op config element. We can always print a warning to the log or stderr. (In reply to Mark Thomas from comment #8) > Moving to enhancement. Does the move to enhancement mean anything (e.g. that it's OK to implement)? If so then I'd be happy to patch it, but if there's still a strong objection then let's close the ticket so that we don't spend more time on it.
The move to enhancement just signifies that it is being treated as an enhancement rather than a bug as they have different expectations when it comes to speed of resolution. I continue to be of the view that it is wrong to make an assumption about why the error occurred and auto-correct based on that potentially invalid assumption. Better, in my view, to have a failed deployment that a system admin has to deal with now than 404s presented to users at some point in the future.
Closing this as WONTFIX per discussion in comments, so we can focus on bigger and better things