Hello team, Thanks, everyone for your inputs. I have attached the patch on the ticket OFBIZ-10539 <https://issues.apache.org/jira/browse/OFBIZ-10539> to fix the issue.
Kindly have a look into this and let me know if there are any additional inputs. On Mon, Aug 27, 2018 at 11:20 AM Ritesh Kumar < [email protected]> wrote: > Thanks, Girish for clearly explaining my concern and everyone for your > inputs. > > I have created the ticket ( OFBIZ-10539 > <https://issues.apache.org/jira/browse/OFBIZ-10539>). Soon, I will be > providing the solution patch for review. > > On Sun, Aug 26, 2018 at 3:36 PM Michael Brohl <[email protected]> > wrote: > >> OFBiz handles multiple request parameters with the same name well if you >> use POST requests. >> >> For GET requests, there seem to be no standard for multiple parameters >> with the same name, see [1]. I thinks it would be good to investigate >> further and see if we can fix this. >> >> I don't see a problem with REST here, the data will most probably >> transferred in some kind of JSON structure. >> >> [1] >> >> https://stackoverflow.com/questions/24059773/correct-way-to-pass-multiple-values-for-same-parameter-name-in-get-request#24728298 >> >> Regards, >> >> Michael >> >> >> Am 26.08.18 um 07:09 schrieb Taher Alkhateeb: >> > Hmmm, based on Girish's feedback I think perhaps the next step should >> be to >> > open a JIRA and further investigate. Thinking about it some more, maybe >> > this would have an impact on the REST API project we're working on. >> > >> > On Sun, Aug 26, 2018, 7:57 AM Girish Vasmatkar < >> > [email protected]> wrote: >> > >> >> 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 >> >>>>>>>>> >> >>>>>>>>> >> >> >>
