Thanks Girish for your detailed analysis!
Jacques
Le 26/08/2018 à 06:57, Girish Vasmatkar a écrit :
Hi Ritesh
It does look like an issue to me.
I believe (correct me if I am wrong) it is not so much about whether GET is
appropriate here, it is more about that the framework is unable to handle
multiple request parameters with same name, which is a common case when we
talk about multiple check boxes on a form representing a single entity. The
fact that GET is doing it's job correctly when the user is logged in (means
when you change the method from POST to GET and the user is already logged
in) and not so much when the user is logged out and the request is made via
bookmark, shows that the code is not working properly.
Then, there is also an issue with URL encoding and decoding that becomes
apparent with executing your scenario. Whether it is correct to change the
form method is arguable, but the code should be able to handle it. If a
form were to be designed with GET method and the only elements present on
the form are two check boxes and a text field and if you were to select
both check boxes and have value with spaces in the text box, this scenario
would still fail.
I think the simplest approach would have been to just store query string
(request.getQueryString()) in the session attribute instead of Map (but I
think it was well pondered upon to use Map) and then just redirecting to
the saved URL once the user loges in. I did it on my local workstation and
it just worked perfectly. May be one reason why a map was used instead of
storing query string was to handle unencoded requests coming from browsers
such as IE. I may be wrong so please correct me if anyone has an idea
around this piece of code as to why Map was used instead of storing the
query string in session to be used later on.
If you can do something to fix the existing code, that would be better
approach, IMO.
May be it is not a major issue but certainly worthy of having a dedicated
JIRA for. Everybody, please chime in and provide your thoughts.
Thanks and Best regards,
Girish Vasmatkar
HotWax Systems
On Sat, Aug 25, 2018 at 8:03 PM Taher Alkhateeb <[email protected]>
wrote:
Okay, I understand this issue. I don't think it is possible to
abstract away a complex search screen with http GET method for
bookmarks. The performFind service is quite complex and it is
difficult to replicate the requirements using GET. GET is not designed
to handle multiple languages, spaces, and other peculiarities that are
needed for such a screen to work.
There are multiple solutions that I can see here. One of them is
simply to create a new entity, let's call it SearchFilter, that saves
search parameters, which can be applied later on. Either way, you need
to customize, your problem is not OFBiz, your problem is http GET
limitations.
On Fri, Aug 24, 2018 at 12:35 PM Ritesh Kumar
<[email protected]> wrote:
Using the POST method does not append form data to the URL, i.e, the
parameters will not be visible in the URL.
For example, take a Find Screen (say, FindWorkEffort) which send data
through a form with POST method. Apply some filters (say, status). No
applied filters appear in the URL. Bookmark this page. Next time when I
open this bookmark, those applied filters will not be there as this page
is
being rendered using data from the URL and since the applied filters were
not there in the bookmarked URL, this page is rendered without the
applied
filters. That is why I used GET form method so that I am able to get the
page with applied filters when I open a bookmarked page.
The bug here is (supposing the GET method is used)
1. On opening the bookmark, the page is rendered with double encoding (if
the value had a space character initially, the space character was
already
encoded into '+' in the URL and when this bookmark is opened, this '+' is
again encoded).
2. Suppose the bookmarked URL had multiple values from the same filter
(say, Cancelled and Declined status), it renders with just one of the
statutes applied. It is because the request handler prepares a Map of
parameters from the query string and as is the property of Map to replace
the old value if a new value is being added with the same key (in this
example, first Cancelled status is put in this Map and then Declined),
only
Declined status is put in this Map.
Hope, this clears the confusion. I will be happy to provide more
information if needed.
On Fri, Aug 24, 2018 at 1:46 PM Taher Alkhateeb <
[email protected]>
wrote:
Not enough information. What happens exactly? What is the bug? What do
you
mean by it does not let us do that?
On Fri, Aug 24, 2018, 11:09 AM Ritesh Kumar <
[email protected]>
wrote:
Hello Taher,
Changing form method to GET is just to make the query parameters
visible
in
the URL so that a user is able to bookmark or share it. Using the
POST
method does not let us do that.
On Fri, Aug 24, 2018 at 11:54 AM Taher Alkhateeb <
[email protected]>
wrote:
Why did you change the method to GET?
On Fri, Aug 24, 2018, 9:20 AM Ritesh Kumar <
[email protected]
wrote:
Just to put my point more clearly, let me add the steps to
generate
the
above-mentioned case. Please refer demo-trunk
<https://demo-trunk.ofbiz.apache.org/webtools/control/main>.
1. Open this link, FindWorkEffort
<
https://demo-trunk.ofbiz.apache.org/workeffort/control/FindWorkEffort
.
Find Work Effort screen will be rendered.
2. Inspect and change the form method to "GET".
3. Apply any of the two statuses (say, Cancelled and Declined).
Click
on
Find.
4. Records will be fetched according to the applied filters.
5. Check the URL. Cancelled and Declined statuses must be there
in
the
URL.
6. Bookmark this page and log out.
7. Now, open the bookmark.
8. The login page will be rendered. Check the URL here. It will
be
the
same
as it was when the page was being bookmarked.
9. Type in the credentials and log in.
10. The result may be different. Check the URL. One of the
statuses
is
gone.
Due to business requirement, I need to show query parameters in
the
URL
so
that the user is able to bookmark the page. And, we normally
pass Id
in
the
parameters, but, due to some reason, I may have to pass values
with
space
characters.
I hope, this demo puts forth my concern.
On Thu, Aug 23, 2018 at 6:27 PM Ritesh Kumar <
[email protected]>
wrote:
Hello All,
I faced an issue while trying to open a bookmarked page with
OFBiz.
Suppose, the URL of this bookmarked page contains a parameter
with
multiple values and the value may have space character. The
query
string
in
the URL looks somewhat like this
"?categoryHierarchy=3%2FCompany+Catalog%2FBrowse+Root%2FCloths%2FMen%2F"&statusId=approved&statusId=created".
The "%2F" and "+" are encoded value of "/", a separator and
space
character respectively. The status id parameter appears twice
and
the
category hierarchy value has space character.
The user is logged out at this instance and this bookmarked
page is
opened. Since the user is not logged in, the login page is
rendered.
I
feed
in the credentials and the intended URL is hit. Here, I do not
get
the
required result.
When I check the URL, the parameter with multiple values just
has
the
last
value of the list and "+" is encoded into "%2B". The URL now is
"?categoryHierarchy=3%2FCompany%2BCatalog%2FBrowse%2BRoot%2FCloths%2FMen%2F"&statusId==created."
I did some digging and found out that LoginWorker.checkLogin()
comes
into
action and what it does is that it creates a new session object
(because
the previous session becomes invalid) and in the session
object, it
puts
the previous URL parameters. This previous URL parameters are
fetched
using
UtilHttp.getUrlOnlyParameterMap(request) which internally calls
getQueryStringOnlyParameterMap(). This method returns a map by
breaking
the
query string into key and value pair. A map can not have
duplicate
keys
(in
this case removes the approved status) and the value is not
decoded
before
putting it into the map ('+' is not decoded). This map is then
used
to
create an encoded ('+' is encoded into '%2B' ) redirect target
and
then
callRedirect() is called on this new redirect target, ending up
with
unintended URL (inside RequestHandler.doRequest()).
I could resolve this issue by decoding the already encoded
value
before
putting it into the Map and if the key is already present in
the
Map,
it
must create a list of the values.
Am I missing something or is this really a bug and needs to be
addressed
OOTB?
If this is a bug, is proposed solution the right one?
--
Best,
Ritesh Kumar
