WW-5548 Defines proper request attributes when forwarding or including final path#1265
WW-5548 Defines proper request attributes when forwarding or including final path#1265lukaszlenart merged 3 commits intomainfrom
Conversation
lukaszlenart
requested review from
aleksandr-m,
cnenning,
Copilot,
kusalk,
rgielen,
sdutry and
yasserzamani
There was a problem hiding this comment.
Pull Request Overview
This PR updates the handling of request attributes when forwarding or including a resource to align with the Jakarta Servlet 6.0 specification. Key changes include:
- Updating test cases in ServletDispatcherResultTest.java to validate the proper request attributes for forward and include scenarios.
- Refactoring ServletDispatcherResult.java to remove the conditional setting of FORWARD_SERVLET_PATH based on query parameters and instead set all required forwarding attributes.
- Introducing analogous attribute settings for include operations to comply with the specification.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/test/java/org/apache/struts2/result/ServletDispatcherResultTest.java | Updated tests to assert correct request attributes for forwarding and including resources. |
| core/src/main/java/org/apache/struts2/result/ServletDispatcherResult.java | Refactored attribute assignment for forwarding and including to align with the Jakarta Servlet 6.0 specification. |
core/src/main/java/org/apache/struts2/result/ServletDispatcherResult.java
Outdated
Show resolved
Hide resolved
MFAshby
left a comment
MFAshby
left a comment
There was a problem hiding this comment.
Thanks very much for your swift response to this issue! It's very much appreciated.
Please see inline comments regarding some issues I still see.
| request.setAttribute("struts.view_uri", finalLocation); | ||
| request.setAttribute("struts.request_uri", request.getRequestURI()); | ||
|
|
||
| // These attributes are required when forwarding to another servlet, see specification: |
There was a problem hiding this comment.
I think setting these is the responsibility of the servlet container e.g. tomcat, inside the implementation of the forward method. We should not have to do it here at all.
| request.setAttribute(RequestDispatcher.FORWARD_MAPPING, request.getHttpServletMapping()); | ||
| request.setAttribute(RequestDispatcher.FORWARD_REQUEST_URI, request.getRequestURI()); | ||
| request.setAttribute(RequestDispatcher.FORWARD_CONTEXT_PATH, request.getContextPath()); | ||
| request.setAttribute(RequestDispatcher.FORWARD_SERVLET_PATH, request.getServletPath()); |
There was a problem hiding this comment.
I think this is missing a guard to set the attribute only if it is not already set. There can be multiple forwards, and this attribute should contain the original request as far as I understand.
Per the other comment I think this is redundant, the servlet container should be setting these properties and we should not touch them.
…by Servlet container
|
MFAshby
left a comment
MFAshby
left a comment
There was a problem hiding this comment.
Fabulous, thank you very much! 😀
|
@MFAshby thanks a lot for review, you were right and these attributes should be controlled by the container. |
|
This change broke integration with Sitemesh if you want to decorate the final JSP, see the example |
2 participants
WW-5548