[jira] Updated: (PB-109) Problem with the request wrapper (user action is not considered)

["Ate Douma (JIRA)" <br...@portals.apache.org>]

     [ 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-unsubscribe@portals.apache.org
For additional commands, e-mail: bridges-dev-help@portals.apache.org