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 <slidingfilame...@gmail.com> 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 > <ritesh.ku...@hotwaxsystems.com> 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 < > slidingfilame...@gmail.com> > > 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 < > > > ritesh.ku...@hotwaxsystems.com> > > > 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 < > > > > slidingfilame...@gmail.com> > > > > wrote: > > > > > > > > > Why did you change the method to GET? > > > > > > > > > > On Fri, Aug 24, 2018, 9:20 AM Ritesh Kumar < > > > > ritesh.ku...@hotwaxsystems.com > > > > > > > > > > > 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 < > > > > > > ritesh.ku...@hotwaxsystems.com> > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
