[
https://issues.apache.org/jira/browse/PB-109?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ate Douma updated PB-109:
-------------------------
Priority: Minor (was: Major)
Hi Ali,
I took a brief look at your patch and thereafter tried to follow the reasoning
from the reported PC-512 issue on exoplatform.
Well, first of all, this patch itself IMO is a completely incorrect/invalid
solution for whatever assumed "bug" within the Struts Bridge:
a) In the first part of the patch you are copying every incoming action
parameter to a response render parameter, *before* even Struts has been invoked.
That truly doesn't make sense to me at all as the actual target behavior of the
portlet (Struts) hasn't even started, so how can you upfront determine if that
is the logical/right thing to do?
(mis)Using render parameters that way really is a very bad pattern imo, and one
which "pollutes" the (target) idempotent portlet render url big time.
Even if in *your* specific use-case (some portlet from the todo-struts.war)
this would make sense somehow, it clearly cannot be forced upon a generic
portlet-bridge framework which will cause every other StrutsPortlet having to
deal with this as well.
b) In the second part of the patch you are manually parsing possible query
string parameters (for a target Struts URL) and (trying to) inject them in the
current HttpServletRequest(Wrapper) to be used for the include call.
This definitely should not be needed at all as by the Servlet Spec SRV.8.4.1
"The request dispatching mechanism is responsible for aggregating query string
parameters when forwarding or including requests."
This means, this already should be done for you, no need to (try to) do this
yourself.
If this isn't done (properly) on the exoplatform container (or by the
underlying web container, e.g. JBoss), you've hit a (*very* serious) bug in
their implementation, which should be fixed there, not hacked around here.
Additionally, trying to modify the current HttpServletRequest(Wrapper)
parameterMap directly as you do in your patch is a outside the servlet API and
bound to fail on (web) containers which properly enforce the servlet spec.
The servlet API (javadoc) for ServletRequest.getParameterMap() clearly states
that it returns an immutable java.util.Map (meaning: you should assume it to be
immutable).
While your code might work in some web containers (e.g. those not enforcing
this requirement), it will blow up on containers who do enforce this.
As such you can understand I really cannot accept and apply this patch.
With regards to the reason for this patch, I'm completely clueless.
As I said above, I tried reading up on the PC-512 issue but couldn't make much
sense out of it (note: I'm not a user of exoplatform nor familiar with their
codebase).
I noticed there was some reference to another issue CCP-296 but I couldn't get
access to it without having to login. But as I don't have an account for
exoplatform I stopped pursuing the issue further.
And, as the todo-struts.war attached to PC-512 didn't come with (Java) sources
I also couldn't review the intended behavior for all this.
My current assessment is: you either have hit on one or more (serious)
container bug(s) in exoplatform/jboss, or your portlet itself is not properly
coded against the portlet spec / StrutsBridge framework interaction.
I will keep this issue open for a while longer but downgrade it to status Minor.
If you can provide additional information (please: here in this issue, not just
at exoplatform JIRA) and concrete explanation how the Struts Bridge causing
(part of) the problem,
of course I'll review and if applicable accept a proper patch.
But do make sure such a new patch is generic and useful for all StrutsPortlet
usages and not only a workaround for one single use-case.
Regards, Ate
> Problem with the request wrapper (user action is not considered)
> ----------------------------------------------------------------
>
> Key: PB-109
> URL: https://issues.apache.org/jira/browse/PB-109
> Project: Portals Bridges
> Issue Type: Bug
> Components: struts
> Affects Versions: 1.0.4
> Reporter: Ali Hamdi
> Priority: Minor
>
> We found a bug in Struts Portals bridge reported here :
> http://jira.exoplatform.org/browse/PC-512
> The fix done by eXo developer is here :
> http://jira.exoplatform.org/secure/attachment/25968/diff.txt
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: bridges-dev-unsubscr...@portals.apache.org
For additional commands, e-mail: bridges-dev-h...@portals.apache.org
