OFBiz
  1. OFBiz
  2. OFBIZ-2628

No Url encoding for get parameters

    Details

      Description

      Let's say I want to create a new category which ID is DVD+R
      Creation is ok through the admin interface, but whenever i want to access this category, the get parameter productCategoryId=.... of the url is wrong as it's not url encoded, so we have :
      https://localhost:8443/catalog/control/EditCategory?productCategoryId=DVD+R instead of
      https://localhost:8443/catalog/control/EditCategory?productCategoryId=DVD%2BR

      Both <@ofbizUrl> tag and menu widgets are not encoding get parameters.

      Way to correct :
      framework/webapp/src/org/ofbiz/webapp/control/RequestHandler.java modify the makeLink function
      framework/widget/src/org/ofbiz/widget/WidgetWorker.java : function buildhyperlinkUrl need to use defaultWebEncoder.encodeForURL instead of simpleEncoder

      Beofre i do the fix, can a core developper let me know about possible side effects

        Activity

        Patrick Antivackis created issue -
        Patrick Antivackis made changes -
        Field Original Value New Value
        Description Let's say I want to create a new category which ID is DVD+R
        Creation is ok through the admin interface, but whenever i want to access this category, the get parameter productCategoryId=.... of the url is wrong as it's not url encoded, so we have :
        https://localhost:8443/catalog/control/EditCategory?productCategoryId=DVD&#43;R instead of
        https://localhost:8443/catalog/control/EditCategory?productCategoryId=DVD%2BR

        Both <@ofbizUrl> tag and menu widgets are not encoding get parameters.

        Way to correct :
        framework/webapp/src/org/ofbiz/webapp/control/RequestHandler.java modify the makeLink function
        framework/widget/src/org/ofbiz/widget/WidgetWorker.java : need to use defaultWebEncoder.encodeForURL instead of simpleEncoder

        Beofre i do the fix, can a core developper let me know about possible side effects
        Let's say I want to create a new category which ID is DVD+R
        Creation is ok through the admin interface, but whenever i want to access this category, the get parameter productCategoryId=.... of the url is wrong as it's not url encoded, so we have :
        https://localhost:8443/catalog/control/EditCategory?productCategoryId=DVD&#43;R instead of
        https://localhost:8443/catalog/control/EditCategory?productCategoryId=DVD%2BR

        Both <@ofbizUrl> tag and menu widgets are not encoding get parameters.

        Way to correct :
        framework/webapp/src/org/ofbiz/webapp/control/RequestHandler.java modify the makeLink function
        framework/widget/src/org/ofbiz/widget/WidgetWorker.java : function buildhyperlinkUrl need to use defaultWebEncoder.encodeForURL instead of simpleEncoder

        Beofre i do the fix, can a core developper let me know about possible side effects
        Hide
        Patrick Antivackis added a comment -

        This is a fix for the widget control in order to Url encode the value of parameters.

        Show
        Patrick Antivackis added a comment - This is a fix for the widget control in order to Url encode the value of parameters.
        Patrick Antivackis made changes -
        Attachment OFBIZ-2628-widget.patch [ 12411571 ]
        Hide
        Patrick Antivackis added a comment -

        Concerning OfbizUrl patch, I really need inputs from a core developper, as the update of the makeLink function is in fact IMO not the right way to solve this issue. I do think that the use of $

        {....?url}

        freemarker function should be the right way to create url. But it's quite a wide task to update all ofbiz ftl
        Any opinion ?

        Show
        Patrick Antivackis added a comment - Concerning OfbizUrl patch, I really need inputs from a core developper, as the update of the makeLink function is in fact IMO not the right way to solve this issue. I do think that the use of $ {....?url} freemarker function should be the right way to create url. But it's quite a wide task to update all ofbiz ftl Any opinion ?
        Hide
        Jacques Le Roux added a comment -

        Hi Patrick,

        Your patch looks good, though it's not an habit in OFBiz to put reference to Jira issues in code. This afterall would add a lot of useless informations, we try to keep the code as clean as possible.

        I had a look at http://freemarker.sourceforge.net/docs/ref_builtins_string.html#ref_builtin_url and http://freemarker.sourceforge.net/docs/pgui_misc_charset.html, but I'd still prefer to make the change in makeLink (I suppose you mean in the "encode section") as

        1. it's used widely and not only in OfbizUrlTransform (look for references in Eclipse)
        2. as you said putting ?url in templates would be wide and hazardous task (I guess I did not check, using regexp replacement in Eclipse you can do miracles)

        My 2 cts

        Show
        Jacques Le Roux added a comment - Hi Patrick, Your patch looks good, though it's not an habit in OFBiz to put reference to Jira issues in code. This afterall would add a lot of useless informations, we try to keep the code as clean as possible. I had a look at http://freemarker.sourceforge.net/docs/ref_builtins_string.html#ref_builtin_url and http://freemarker.sourceforge.net/docs/pgui_misc_charset.html , but I'd still prefer to make the change in makeLink (I suppose you mean in the "encode section") as it's used widely and not only in OfbizUrlTransform (look for references in Eclipse) as you said putting ?url in templates would be wide and hazardous task (I guess I did not check, using regexp replacement in Eclipse you can do miracles) My 2 cts
        Hide
        Jacques Le Roux added a comment -

        Hi PAtrick,

        I was ready to commit your patch but I'd prefer to commit the changes to makelink in the same shoot.
        Could you please update your patch with the change you propose ?
        Or have you some concerns about the way you propose to change makelink ?

        Show
        Jacques Le Roux added a comment - Hi PAtrick, I was ready to commit your patch but I'd prefer to commit the changes to makelink in the same shoot. Could you please update your patch with the change you propose ? Or have you some concerns about the way you propose to change makelink ?
        Hide
        Patrick Antivackis added a comment - - edited

        Jacques,
        In fact I tried, as you proposed, to create a bug fix in the RequestHandler.makelink, but i stumble onto the following question:
        Why in the <@ofbizurl> tag, the & to separate parameters in the query string is written as & amp; like in the applications/product/webapp/catalog/find/sidecatalogs.ftl line 29 : <@ofbizUrl>EditCategory?CATALOG_TOP_CATEGORY=$

        {prodCatalogCategory.productCategoryId}& amp;productCategoryId=${prodCatalogCategory.productCategoryId}

        </@ofbizUrl> and like in many other ftl templates (maybe all)
        Is it the mandatory way to do it and is it writtent clearly somewhere ? I ask this because,personnaly i always used only the & without the & in the ofbizurl tag.

        So if it is the official way to write queryString, i can do the patch, else if it is not, i can't because the queryString would contains '&' that doesn't mean 'separation of parameters', like dvd+r is in fact output dvd& #43;r by freemarker $

        {product.productId}
        Show
        Patrick Antivackis added a comment - - edited Jacques, In fact I tried, as you proposed, to create a bug fix in the RequestHandler.makelink, but i stumble onto the following question: Why in the <@ofbizurl> tag, the & to separate parameters in the query string is written as & amp; like in the applications/product/webapp/catalog/find/sidecatalogs.ftl line 29 : <@ofbizUrl>EditCategory?CATALOG_TOP_CATEGORY=$ {prodCatalogCategory.productCategoryId}& amp;productCategoryId=${prodCatalogCategory.productCategoryId} </@ofbizUrl> and like in many other ftl templates (maybe all) Is it the mandatory way to do it and is it writtent clearly somewhere ? I ask this because,personnaly i always used only the & without the & in the ofbizurl tag. So if it is the official way to write queryString, i can do the patch, else if it is not, i can't because the queryString would contains '&' that doesn't mean 'separation of parameters', like dvd+r is in fact output dvd& #43;r by freemarker $ {product.productId}
        Hide
        Scott Gray added a comment - - edited

        We should be using

        &amp;

        to separate parameters since we target xhtml

        EDIT: jira turned my & amp; into &

        Show
        Scott Gray added a comment - - edited We should be using &amp; to separate parameters since we target xhtml EDIT: jira turned my & amp; into &
        Hide
        Patrick Antivackis added a comment -

        You are right.
        Thank you Scott for this efficient answer.

        Show
        Patrick Antivackis added a comment - You are right. Thank you Scott for this efficient answer.
        Patrick Antivackis made changes -
        Attachment OFBIZ-2628-widget.patch [ 12411571 ]
        Hide
        Patrick Antivackis added a comment - - edited

        Trunk patch at revision 830787
        Patch not good, need to rework it

        Show
        Patrick Antivackis added a comment - - edited Trunk patch at revision 830787 Patch not good, need to rework it
        Patrick Antivackis made changes -
        Attachment Patch-OFBIZ-2628.txt [ 12423509 ]
        Patrick Antivackis made changes -
        Attachment Patch-OFBIZ-2628.txt [ 12423509 ]
        Gavin made changes -
        Workflow jira [ 12466282 ] OFbiz Workflow [ 12505899 ]
        Hide
        Jacques Le Roux added a comment -

        I was looking at this issue by chance (actually 1st in most important list) and trying to associate DVD+R a parent rollup I got this error

        ERROR: Could not complete the Add ProductCategory to Category file:/home/ofbiz/trunk/applications/product/script/org/ofbiz/product/category/CategoryServices.xml#addProductCategoryToCategory process [problem creating the newEntity value: Error while inserting: [GenericEntity:ProductCategoryRollup][createdStamp,2010-06-04 10:01:45.461(java.sql.Timestamp)][createdTxStamp,2010-06-04 10:01:45.439(java.sql.Timestamp)][fromDate,2010-06-04 11:00:40.44(java.sql.Timestamp)][lastUpdatedStamp,2010-06-04 10:01:45.461(java.sql.Timestamp)][lastUpdatedTxStamp,2010-06-04 10:01:45.439(java.sql.Timestamp)][parentProductCategoryId,CATALOG1(java.lang.String)][productCategoryId,DVD R(java.lang.String)] (SQL Exception while executing the following:INSERT INTO OFBIZ.PRODUCT_CATEGORY_ROLLUP (PRODUCT_CATEGORY_ID, PARENT_PRODUCT_CATEGORY_ID, FROM_DATE, THRU_DATE, SEQUENCE_NUM, LAST_UPDATED_STAMP, LAST_UPDATED_TX_STAMP, CREATED_STAMP, CREATED_TX_STAMP) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) (INSERT on table 'PRODUCT_CATEGORY_ROLLUP' caused a violation of foreign key constraint 'PROD_CRLP_CURRENT' for key (DVD R). The statement has been rolled back.))]

        Patrick, do you think you will continue to work on this, have you solved it?

        Thanks

        Show
        Jacques Le Roux added a comment - I was looking at this issue by chance (actually 1st in most important list) and trying to associate DVD+R a parent rollup I got this error ERROR: Could not complete the Add ProductCategory to Category file:/home/ofbiz/trunk/applications/product/script/org/ofbiz/product/category/CategoryServices.xml#addProductCategoryToCategory process [problem creating the newEntity value: Error while inserting: [GenericEntity:ProductCategoryRollup] [createdStamp,2010-06-04 10:01:45.461(java.sql.Timestamp)] [createdTxStamp,2010-06-04 10:01:45.439(java.sql.Timestamp)] [fromDate,2010-06-04 11:00:40.44(java.sql.Timestamp)] [lastUpdatedStamp,2010-06-04 10:01:45.461(java.sql.Timestamp)] [lastUpdatedTxStamp,2010-06-04 10:01:45.439(java.sql.Timestamp)] [parentProductCategoryId,CATALOG1(java.lang.String)] [productCategoryId,DVD R(java.lang.String)] (SQL Exception while executing the following:INSERT INTO OFBIZ.PRODUCT_CATEGORY_ROLLUP (PRODUCT_CATEGORY_ID, PARENT_PRODUCT_CATEGORY_ID, FROM_DATE, THRU_DATE, SEQUENCE_NUM, LAST_UPDATED_STAMP, LAST_UPDATED_TX_STAMP, CREATED_STAMP, CREATED_TX_STAMP) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) (INSERT on table 'PRODUCT_CATEGORY_ROLLUP' caused a violation of foreign key constraint 'PROD_CRLP_CURRENT' for key (DVD R). The statement has been rolled back.))] Patrick, do you think you will continue to work on this, have you solved it? Thanks
        Hide
        Jacques Le Roux added a comment -

        This begins to be old, any update? Shoud we not close?

        Show
        Jacques Le Roux added a comment - This begins to be old, any update? Shoud we not close?
        Hide
        Markus M. May added a comment -

        The patch is not attached to this ticket anymore. Would like to help out here, do you still have the patch somewhere, so that the start is easier?

        Show
        Markus M. May added a comment - The patch is not attached to this ticket anymore. Would like to help out here, do you still have the patch somewhere, so that the start is easier?
        Hide
        Markus M. May added a comment -

        Patch to encode URLs in the WidgetWorker as well as in the RequestHandler.

        Tested using the already mentioned DVD+R category, more testing is needed.

        Show
        Markus M. May added a comment - Patch to encode URLs in the WidgetWorker as well as in the RequestHandler. Tested using the already mentioned DVD+R category, more testing is needed.
        Markus M. May made changes -
        Sascha Rodekamp made changes -
        Assignee Sascha Rodekamp [ sascha ]
        Hide
        Sascha Rodekamp added a comment - - edited

        Hi Markus, thanks for your patch.

        But when i create a new category "DVD+R" the widget encoder encodes it to "DVD+R" not to "DVD%2BR".
        It would be great if you can have a second look at this.

        BTW do we have to port a patch to the Release Branch 4.0 is it still supported?

        Have a good day
        Sascha

        Show
        Sascha Rodekamp added a comment - - edited Hi Markus, thanks for your patch. But when i create a new category "DVD+R" the widget encoder encodes it to "DVD+R" not to "DVD%2BR". It would be great if you can have a second look at this. BTW do we have to port a patch to the Release Branch 4.0 is it still supported? Have a good day Sascha
        Hide
        Jacopo Cappellato added a comment -

        Sascha,

        release 4.0 is actually very old and imo we should not worry about it (unless there is a real interest in the community). I will start a thread to discuss this (and 09.04).

        Show
        Jacopo Cappellato added a comment - Sascha, release 4.0 is actually very old and imo we should not worry about it (unless there is a real interest in the community). I will start a thread to discuss this (and 09.04).
        Hide
        Sascha Rodekamp added a comment -

        I would suggest using the URLEncoder to fix this issue (see attached files).
        The URLEncoder does a right encoding for the url with a special character.

        Any comments to this solution?

        Regards

        Show
        Sascha Rodekamp added a comment - I would suggest using the URLEncoder to fix this issue (see attached files). The URLEncoder does a right encoding for the url with a special character. Any comments to this solution? Regards
        Sascha Rodekamp made changes -
        Hide
        Jacques Le Roux added a comment -

        Hi Sascha,

        It works here. But I read in the autocompletion JavaDoc (using 1.6.22 in Eclipse on Windows XP)

        Note: The World Wide Web Consortium Recommendation states that UTF-8 should be used. Not doing so may introduce incompatibilites.

        This is best explained here http://www.w3.org/TR/html40/appendix/notes.html#non-ascii-chars

        Though I did not find a so affirmative advice in the Java 6 API doc, I also read

        The recommended encoding scheme to use is UTF-8. However, for compatibility reasons, if an encoding is not specified, then the default encoding of the platform is used.

        So I think we should rather use Charset.forName("UTF-8").displayName() instead of Charset.defaultCharset().displayName(). AFAIK, UTF-8 should fits with all server and clients types...

        Show
        Jacques Le Roux added a comment - Hi Sascha, It works here. But I read in the autocompletion JavaDoc (using 1.6.22 in Eclipse on Windows XP) Note: The World Wide Web Consortium Recommendation states that UTF-8 should be used. Not doing so may introduce incompatibilites. This is best explained here http://www.w3.org/TR/html40/appendix/notes.html#non-ascii-chars Though I did not find a so affirmative advice in the Java 6 API doc , I also read The recommended encoding scheme to use is UTF-8. However, for compatibility reasons, if an encoding is not specified, then the default encoding of the platform is used. So I think we should rather use Charset.forName("UTF-8").displayName() instead of Charset.defaultCharset().displayName(). AFAIK, UTF-8 should fits with all server and clients types...
        Hide
        Jacques Le Roux added a comment -

        Forgot to mention that the changes in WidgetWorker.java are OK with me

        Show
        Jacques Le Roux added a comment - Forgot to mention that the changes in WidgetWorker.java are OK with me
        Hide
        Sascha Rodekamp added a comment -

        Hi Jacques,
        yes i think hard coding UTF-8 is sufficient, first i thought of getting it from an property file, but as you mentioned the World Wide Web Consortium recommended also UTF-8.

        Thanks for reviewing
        Have a good day
        Sascha

        Show
        Sascha Rodekamp added a comment - Hi Jacques, yes i think hard coding UTF-8 is sufficient, first i thought of getting it from an property file, but as you mentioned the World Wide Web Consortium recommended also UTF-8. Thanks for reviewing Have a good day Sascha
        Hide
        Sascha Rodekamp added a comment -

        Thanks Markus for the initial Patch i committed my modified version in:

        Trunk @Rev 1297006
        10.04 @Rev 1297030
        11.04 @Rev 1297031

        Show
        Sascha Rodekamp added a comment - Thanks Markus for the initial Patch i committed my modified version in: Trunk @Rev 1297006 10.04 @Rev 1297030 11.04 @Rev 1297031
        Sascha Rodekamp made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s Release Branch 10.04 [ 12314832 ]
        Fix Version/s Release Branch 11.04 [ 12316420 ]
        Fix Version/s SVN trunk [ 12311928 ]
        Resolution Fixed [ 1 ]
        Hide
        Sascha Rodekamp added a comment -

        The patch breaks the http --> https redirect. Which could be seen when using the ecommerce login button.

        Show
        Sascha Rodekamp added a comment - The patch breaks the http --> https redirect. Which could be seen when using the ecommerce login button.
        Sascha Rodekamp made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Sascha Rodekamp added a comment -

        fixed the redirect bug. The encoding in the RequestHandler was obsolete and causes this error.

        The new patch is committed in
        Trunk @Rev1298454
        11.04 @Rev1298455
        10.04 @Rev1298456

        Show
        Sascha Rodekamp added a comment - fixed the redirect bug. The encoding in the RequestHandler was obsolete and causes this error. The new patch is committed in Trunk @Rev1298454 11.04 @Rev1298455 10.04 @Rev1298456
        Sascha Rodekamp made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Hide
        Markus M. May added a comment -

        Hello, I have already written a short comment on OFBIZ-2837, this does not seem to be resolved, we still face this issue in the ecommerce application. We are using ofbiz trunk.

        Show
        Markus M. May added a comment - Hello, I have already written a short comment on OFBIZ-2837 , this does not seem to be resolved, we still face this issue in the ecommerce application. We are using ofbiz trunk.
        Hide
        Sascha Rodekamp added a comment -

        Hey Markus, ok no problem. I will reopen the issue,
        and check if there is a relation to the issue you metioned and maybe we get closer to a solution .

        Show
        Sascha Rodekamp added a comment - Hey Markus, ok no problem. I will reopen the issue, and check if there is a relation to the issue you metioned and maybe we get closer to a solution .
        Sascha Rodekamp made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Jacques Le Roux added a comment -

        Sascha, (or anyone else)

        If ever you get to fix this, please check if it fixes also OFBIZ-2837 anc close it

        TIA

        Show
        Jacques Le Roux added a comment - Sascha, (or anyone else) If ever you get to fix this, please check if it fixes also OFBIZ-2837 anc close it TIA
        Hide
        Sascha Rodekamp added a comment -

        Hi Markus, hi Jacques

        i got also a problem with german umlauts when loading a category with the name "Bäume". The 'ä' is not encoded correctly.

        But the issue OFBIZ-2837 has another problem. I tested this with the keywordsearchbox in the catalog screens.
        The Reqeust Handler calls the render process which calls a method "UtilHttp.getParameterMap", this method extracts the parameters from the request object. At the end of this all parameter values are canonicalized with the defaultWebEncoder. The result is that the "02S%000" search string becomes "02S 0". We do this encoding for security reasons, i'm wondering what we can do here to provide correct search strings and don't open a security hole?
        Any suggestions?

        Have a good day,
        Sascha

        Show
        Sascha Rodekamp added a comment - Hi Markus, hi Jacques i got also a problem with german umlauts when loading a category with the name "Bäume". The 'ä' is not encoded correctly. But the issue OFBIZ-2837 has another problem. I tested this with the keywordsearchbox in the catalog screens. The Reqeust Handler calls the render process which calls a method "UtilHttp.getParameterMap", this method extracts the parameters from the request object. At the end of this all parameter values are canonicalized with the defaultWebEncoder. The result is that the "02S%000" search string becomes "02S 0". We do this encoding for security reasons, i'm wondering what we can do here to provide correct search strings and don't open a security hole? Any suggestions? Have a good day, Sascha
        Hide
        Sascha Rodekamp added a comment -

        Setting the encoding to ISO-8859-1 solves the umlauts problem.

        Anyone an idea why the URL UTF-8 encoding doesn't work properly?
        Seems that the browser does some internal decoding here?! Moving away from UTF-8 isn't a good idea!

        Anyway changing the encoding to ISO-8859-1 makes it, for me, possible to create and use categories like: "Bücher", "D+Ä" ...

        WidgetWorker.java line: 382
        returnValue = URLEncoder.encode(retVal.toString(), Charset.forName("ISO-8859-1").displayName());
        
        Show
        Sascha Rodekamp added a comment - Setting the encoding to ISO-8859-1 solves the umlauts problem. Anyone an idea why the URL UTF-8 encoding doesn't work properly? Seems that the browser does some internal decoding here?! Moving away from UTF-8 isn't a good idea! Anyway changing the encoding to ISO-8859-1 makes it, for me, possible to create and use categories like: "Bücher", "D+Ä" ... WidgetWorker.java line: 382 returnValue = URLEncoder.encode(retVal.toString(), Charset.forName( "ISO-8859-1" ).displayName());
        Hide
        Jacques Le Roux added a comment - - edited

        == MISSED A WORD ==
        I unfortunately have no time to digg more about this at the moment, but I think this link could be helpful http://wiki.apache.org/tomcat/FAQ/CharacterEncoding#Q8 (OFBiz is already doing most of this, did not check all details though)

        Also, not directly related, but it's a "well known issue" that UTF8-encoding does not work for fields sent with GET method using Ajax...

        Show
        Jacques Le Roux added a comment - - edited == MISSED A WORD == I unfortunately have no time to digg more about this at the moment, but I think this link could be helpful http://wiki.apache.org/tomcat/FAQ/CharacterEncoding#Q8 (OFBiz is already doing most of this, did not check all details though) Also, not directly related, but it's a "well known issue" that UTF8-encoding does not work for fields sent with GET method using Ajax...
        Hide
        Wojciech Szymanowski added a comment -

        Hello,

        I am using 11.04 version, and have noticed that CatalinaContainer.createConnector() method does not initialize Connector.URIEncoding property from ContainerConfig class.

        Current code in CatalinaContainer.createConnector() which does not work for URIEncoding property:
        for (ContainerConfig.Container.Property prop: connectorProp.properties.values())

        { connector.setProperty(prop.name, prop.value); }

        When I added following lines:
        if (connectorProp.properties.containsKey("URIEncoding"))

        { connector.setURIEncoding(connectorProp.properties.get("URIEncoding").value); }

        ... it solved the problem.

        Show
        Wojciech Szymanowski added a comment - Hello, I am using 11.04 version, and have noticed that CatalinaContainer.createConnector() method does not initialize Connector.URIEncoding property from ContainerConfig class. Current code in CatalinaContainer.createConnector() which does not work for URIEncoding property: for (ContainerConfig.Container.Property prop: connectorProp.properties.values()) { connector.setProperty(prop.name, prop.value); } When I added following lines: if (connectorProp.properties.containsKey("URIEncoding")) { connector.setURIEncoding(connectorProp.properties.get("URIEncoding").value); } ... it solved the problem.
        Hide
        Sascha Rodekamp added a comment - - edited

        I'm finally back form my holiday.

        Great observation Wojciech, I will check that.

        Edit: The changes work great, I can't identify any side effects.
        I committed the changes in
        Trunk @Rev1390542
        12.04 @Rev1390546
        11.04 @Rev1390544

        Show
        Sascha Rodekamp added a comment - - edited I'm finally back form my holiday. Great observation Wojciech, I will check that. Edit: The changes work great, I can't identify any side effects. I committed the changes in Trunk @Rev1390542 12.04 @Rev1390546 11.04 @Rev1390544
        Sascha Rodekamp made changes -
        Fix Version/s Release Branch 10.04 [ 12314832 ]
        Resolution Fixed [ 1 ]
        Status Reopened [ 4 ] Closed [ 6 ]
        Fix Version/s Release Branch 12.04 [ 12321265 ]
        Hide
        Jacques Le Roux added a comment -

        Thanks for checking Wojciech,

        When I posted my last comment I did not suspect URIEncoding has never been implemented.

        Anyway I merged by hand in R10.04, run tests, got an unrelated errors with testGetInventoryAvailableByFacility

        Assertion failed: [availableToPromiseTotal=507] equals 509 as BigDecimal
        

        and a transformation error when closing while trying to copy to html results

        [junitreport] the file D:\workspace\release10.04\runtime\logs\test-results\TESTS-TestSuites.xml is not a valid testsuite XML document
        [junitreport] Processing D:\workspace\release10.04\runtime\logs\test-results\TESTS-TestSuites.xml to C:\DOCUME~1\JACQUE~1\LOCALS~1\Temp\null279273951
        [junitreport] Loading stylesheet jar:file:/D:/workspace/release10.04/framework/base/lib/ant-junit-1.7.1.jar!/org/apache/tools/ant/taskdefs/optional/junit/xsl/junit-frames.xsl
        [junitreport] Transform time: 859ms
        [junitreport] Deleting: C:\DOCUME~1\JACQUE~1\LOCALS~1\Temp\null279273951
        

        I decided to commit at r1390845 and will see later why we get those

        Show
        Jacques Le Roux added a comment - Thanks for checking Wojciech, When I posted my last comment I did not suspect URIEncoding has never been implemented. Anyway I merged by hand in R10.04, run tests, got an unrelated errors with testGetInventoryAvailableByFacility Assertion failed: [availableToPromiseTotal=507] equals 509 as BigDecimal and a transformation error when closing while trying to copy to html results [junitreport] the file D:\workspace\release10.04\runtime\logs\test-results\TESTS-TestSuites.xml is not a valid testsuite XML document [junitreport] Processing D:\workspace\release10.04\runtime\logs\test-results\TESTS-TestSuites.xml to C:\DOCUME~1\JACQUE~1\LOCALS~1\Temp\null279273951 [junitreport] Loading stylesheet jar:file:/D:/workspace/release10.04/framework/base/lib/ant-junit-1.7.1.jar!/org/apache/tools/ant/taskdefs/optional/junit/xsl/junit-frames.xsl [junitreport] Transform time: 859ms [junitreport] Deleting: C:\DOCUME~1\JACQUE~1\LOCALS~1\Temp\null279273951 I decided to commit at r1390845 and will see later why we get those
        Hide
        Jacques Le Roux added a comment - - edited

        OK, as I thought it went well in Buildbot

        Show
        Jacques Le Roux added a comment - - edited OK, as I thought it went well in Buildbot
        Hide
        Wojciech Szymanowski added a comment - - edited

        Hi,

        There are still 2 problems with encoding parameters:
        a) parameters from hidden fields sending with POST method
        b) parameters sending during "request-redirect" response type

        I prepared 2 patches solving these problems for me. Could you check them?

        Show
        Wojciech Szymanowski added a comment - - edited Hi, There are still 2 problems with encoding parameters: a) parameters from hidden fields sending with POST method b) parameters sending during "request-redirect" response type I prepared 2 patches solving these problems for me. Could you check them?
        Wojciech Szymanowski made changes -
        Hide
        Jacques Le Roux added a comment -

        Let's see...

        Show
        Jacques Le Roux added a comment - Let's see...
        Jacques Le Roux made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Sascha Rodekamp added a comment -

        Hm, the patch "OFBIZ-2628-encoding-hidden-post-params.patch" break the encoding again.

        Show
        Sascha Rodekamp added a comment - Hm, the patch " OFBIZ-2628 -encoding-hidden-post-params.patch" break the encoding again.
        Hide
        Wojciech Szymanowski added a comment -

        Sascha, and which version do you test? I have 11.04 version, but it is quite old (from 2012-05-09 01:24:16)

        Show
        Wojciech Szymanowski added a comment - Sascha, and which version do you test? I have 11.04 version, but it is quite old (from 2012-05-09 01:24:16)
        Hide
        Sascha Rodekamp added a comment -

        I applied the patch on trunk. Which causes the same issue as described in the in the bug description.

        Show
        Sascha Rodekamp added a comment - I applied the patch on trunk. Which causes the same issue as described in the in the bug description.
        Hide
        Wojciech Szymanowski added a comment -

        You are right. So I have to find another solution to fix a problem with encoding POST hidden parameters. I have noticed I have this problem only on forms of "list" type.

        Show
        Wojciech Szymanowski added a comment - You are right. So I have to find another solution to fix a problem with encoding POST hidden parameters. I have noticed I have this problem only on forms of "list" type.
        Hide
        Sascha Rodekamp added a comment -

        Ok no problem, can you give me a short example of your problem that I can reproduce it here on my machine?

        Show
        Sascha Rodekamp added a comment - Ok no problem, can you give me a short example of your problem that I can reproduce it here on my machine?
        Hide
        Wojciech Szymanowski added a comment -

        The problem occurs only on list forms with submit action per row, where tag form is genereted for the submit action. Hidden fields values inside such form tag should not be "url encoded".

        I prepared a patch. Please check it.

        Show
        Wojciech Szymanowski added a comment - The problem occurs only on list forms with submit action per row, where tag form is genereted for the submit action. Hidden fields values inside such form tag should not be "url encoded". I prepared a patch. Please check it.
        Wojciech Szymanowski made changes -
        Hide
        Sascha Rodekamp added a comment -

        Hey,
        thanks for the new patch. The encoding of the parameters works now. But I get NPE's when loading the global decorator on different pages i.e.: https://localhost:8443/content/control/ListWebSiteContent?webSiteId=CmsSite
        The exception occurs in: WidgetWorker.makeHiddenFormLinkForm

        Show
        Sascha Rodekamp added a comment - Hey, thanks for the new patch. The encoding of the parameters works now. But I get NPE's when loading the global decorator on different pages i.e.: https://localhost:8443/content/control/ListWebSiteContent?webSiteId=CmsSite The exception occurs in: WidgetWorker.makeHiddenFormLinkForm
        Hide
        Deepak Dixit added a comment -

        Here is the link to reproduce the hidden form encoding issue:

        https://demo-trunk.ofbiz.apache.org/catalog/control/EditProductAttributes?productId=DEMO-PRODUCT-1

        Add an product attribute "Test Attr" and then try to delete it.

        Show
        Deepak Dixit added a comment - Here is the link to reproduce the hidden form encoding issue: https://demo-trunk.ofbiz.apache.org/catalog/control/EditProductAttributes?productId=DEMO-PRODUCT-1 Add an product attribute "Test Attr" and then try to delete it.
        Hide
        Sascha Rodekamp added a comment -

        Good morning guys,

        I tested that on trunk and 11.04 and it works fine. No problems with creating and deleting an attribute.

        Show
        Sascha Rodekamp added a comment - Good morning guys, I tested that on trunk and 11.04 and it works fine. No problems with creating and deleting an attribute.
        Hide
        Deepak Dixit added a comment -

        Hi Sascha,

        Please add an space in attribute name (like "Test Attr"). I am able to regenerate it over trunk.
        I am getting following error:

        <p>Error running the simple-method: Entity value not found with name: lookedUpValue Method = deleteProductAttribute, File = file:/home/ofbiz/trunk/applications/product/script/org/ofbiz/product/product/ProductServices.xml, Element = <remove-value>, Line 961null</p>

        This is because in delete from url encoding replace space with + symbol.

        <input type="hidden" value="Test+Attribute" name="attrName">

        Show
        Deepak Dixit added a comment - Hi Sascha, Please add an space in attribute name (like "Test Attr"). I am able to regenerate it over trunk. I am getting following error: <p>Error running the simple-method: Entity value not found with name: lookedUpValue Method = deleteProductAttribute, File = file:/home/ofbiz/trunk/applications/product/script/org/ofbiz/product/product/ProductServices.xml , Element = <remove-value>, Line 961null</p> This is because in delete from url encoding replace space with + symbol. <input type="hidden" value="Test+Attribute" name="attrName">
        Hide
        Jacques Le Roux added a comment -

        Hi Sascha,

        DId you try with a white space in name as Deepak suggested? I reproduce locally with "Test Attr"

        2012-09-28 08:19:35,000 (http-bio-0.0.0.0-8443-exec-3) [    TransactionUtil.java:379:WARN ]
        ---- exception report ----------------------------------------------------------
        [TransactionUtil.setRollbackOnly] Calling transaction setRollbackOnly; this stack trace shows where this is happening:
        Exception: java.lang.Exception
        Message: Error in simple-method [delete a ProductAttribute [file:/D:/workspace/ofbizClean/applications/product/script/org/ofbiz/product/product/ProductServices.xml#deleteProductAttribute]]: Error running the
        simple-method: Entity value not found with name: lookedUpValue Method = deleteProductAttribute, File = file:/D:/workspace/ofbizClean/applications/product/script/org/ofbiz/product/product/ProductServices.xml,
        Element = <remove-value>, Line 961null
        ---- stack trace ---------------------------------------------------------------
        

        Looking at lookedUpValue Method = deleteProductAttribute...

        Show
        Jacques Le Roux added a comment - Hi Sascha, DId you try with a white space in name as Deepak suggested? I reproduce locally with "Test Attr" 2012-09-28 08:19:35,000 (http-bio-0.0.0.0-8443-exec-3) [ TransactionUtil.java:379:WARN ] ---- exception report ---------------------------------------------------------- [TransactionUtil.setRollbackOnly] Calling transaction setRollbackOnly; this stack trace shows where this is happening: Exception: java.lang.Exception Message: Error in simple-method [delete a ProductAttribute [file:/D:/workspace/ofbizClean/applications/product/script/org/ofbiz/product/product/ProductServices.xml#deleteProductAttribute]]: Error running the simple-method: Entity value not found with name: lookedUpValue Method = deleteProductAttribute, File = file:/D:/workspace/ofbizClean/applications/product/script/org/ofbiz/product/product/ProductServices.xml, Element = <remove-value>, Line 961null ---- stack trace --------------------------------------------------------------- Looking at lookedUpValue Method = deleteProductAttribute...
        Hide
        Jacques Le Roux added a comment -

        Mmm no way, this is a really simple one

            <simple-method method-name="deleteProductAttribute" short-description="delete a ProductAttribute">
                <entity-one entity-name="ProductAttribute" value-field="lookedUpValue"/>
                <remove-value value-field="lookedUpValue"/>
            </simple-method>
        

        So yes, we have a problem Houston

        Show
        Jacques Le Roux added a comment - Mmm no way, this is a really simple one <simple-method method-name= "deleteProductAttribute" short -description= "delete a ProductAttribute" > <entity-one entity-name= "ProductAttribute" value-field= "lookedUpValue" /> <remove-value value-field= "lookedUpValue" /> </simple-method> So yes, we have a problem Houston
        Hide
        Deepak Dixit added a comment -

        As attrName is part of primary key and due to url encoding space replace with + symbol so lookupValue is not able to find the record.
        Update Product attribute is working fine because there in no encoding in update post form .
        Here is update product attribute form:

        <form name="UpdateProductAttribute_o_0" onsubmit="javascript:submitFormDisableSubmits(this)" class="basic-form" id="UpdateProductAttribute_o_0" action="/catalog/control/updateProductAttribute" method="post" novalidate="novalidate">
          
          <input type="hidden" id="UpdateProductAttribute_productId_o_0" value="DEMO-PRODUCT-1" name="productId">
          <input type="hidden" id="UpdateProductAttribute_attrName_o_0" value="Test Attribute" name="attrName">
          <input type="text" autocomplete="off" id="UpdateProductAttribute_attrValue_o_0" size="25" name="attrValue">
          <input type="text" autocomplete="off" id="UpdateProductAttribute_attrType_o_0" size="25" name="attrType">
          <input type="submit" value="Update" name="submitButton">
        </form>
        

        Here is delete product attribute:

        
        <form name="UpdateProductAttribute_o_0_0_o_deleteLink" onsubmit="javascript:submitFormDisableSubmits(this)" action="/catalog/control/deleteProductAttribute" method="post">
          <input type="hidden" value="DEMO-PRODUCT-1" name="productId">
          <input type="hidden" value="Test+Attribute" name="attrName">
          <input type="hidden" value="0" name="VIEW_INDEX_1">
          <input type="hidden" value="20" name="VIEW_SIZE_1">
        </form>
        
        Show
        Deepak Dixit added a comment - As attrName is part of primary key and due to url encoding space replace with + symbol so lookupValue is not able to find the record. Update Product attribute is working fine because there in no encoding in update post form . Here is update product attribute form: <form name= "UpdateProductAttribute_o_0" onsubmit= "javascript:submitFormDisableSubmits( this )" class= "basic-form" id= "UpdateProductAttribute_o_0" action= "/catalog/control/updateProductAttribute" method= "post" novalidate= "novalidate" > <input type= "hidden" id= "UpdateProductAttribute_productId_o_0" value= "DEMO-PRODUCT-1" name= "productId" > <input type= "hidden" id= "UpdateProductAttribute_attrName_o_0" value= "Test Attribute" name= "attrName" > <input type= "text" autocomplete= "off" id= "UpdateProductAttribute_attrValue_o_0" size= "25" name= "attrValue" > <input type= "text" autocomplete= "off" id= "UpdateProductAttribute_attrType_o_0" size= "25" name= "attrType" > <input type= "submit" value= "Update" name= "submitButton" > </form> Here is delete product attribute: <form name= "UpdateProductAttribute_o_0_0_o_deleteLink" onsubmit= "javascript:submitFormDisableSubmits( this )" action= "/catalog/control/deleteProductAttribute" method= "post" > <input type= "hidden" value= "DEMO-PRODUCT-1" name= "productId" > <input type= "hidden" value= "Test+Attribute" name= "attrName" > <input type= "hidden" value= "0" name= "VIEW_INDEX_1" > <input type= "hidden" value= "20" name= "VIEW_SIZE_1" > </form>
        Hide
        Sascha Rodekamp added a comment -

        Ah ok i only tested with special chars like + and äöü, which works fine. But the a space is really a problem. Let's see what we can do here...

        Show
        Sascha Rodekamp added a comment - Ah ok i only tested with special chars like + and äöü, which works fine. But the a space is really a problem. Let's see what we can do here...
        Hide
        Wojciech Szymanowski added a comment -

        Sascha,
        I cannot reproduce NPE exception in WidgetWorker.makeHiddenFormLinkForm after patch "OFBIZ-2628-encoding-hidden-post-params (list forms)", but I have prepared new patch with checking for null in this situation. Maybe it will help.

        Show
        Wojciech Szymanowski added a comment - Sascha, I cannot reproduce NPE exception in WidgetWorker.makeHiddenFormLinkForm after patch " OFBIZ-2628 -encoding-hidden-post-params (list forms)", but I have prepared new patch with checking for null in this situation. Maybe it will help.
        Wojciech Szymanowski made changes -
        Hide
        Sascha Rodekamp added a comment -

        Hi,
        I changed your patch slightly and in worked.
        The Problem is, that "VIEW_SIZE" is only stored in parameters and not in the context object.
        A simple containsKey on the context object solves the problem

        Working with hidden fields work now for me, could you please so a recheck.
        I created the patch file from trunk.

        Show
        Sascha Rodekamp added a comment - Hi, I changed your patch slightly and in worked. The Problem is, that "VIEW_SIZE" is only stored in parameters and not in the context object. A simple containsKey on the context object solves the problem Working with hidden fields work now for me, could you please so a recheck. I created the patch file from trunk.
        Sascha Rodekamp made changes -
        Hide
        Wojciech Szymanowski added a comment -

        Sascha, your patch also works for me.

        Show
        Wojciech Szymanowski added a comment - Sascha, your patch also works for me.
        Hide
        Sascha Rodekamp added a comment -

        Ok doki.
        I committed the patch in:

        10.04 @Rev1392767
        11.04 @Rev1392768
        12.04 @Rev1392769
        Trunk @Rev1392766

        So we could finally close the issue ?!

        Show
        Sascha Rodekamp added a comment - Ok doki. I committed the patch in: 10.04 @Rev1392767 11.04 @Rev1392768 12.04 @Rev1392769 Trunk @Rev1392766 So we could finally close the issue ?!
        Sascha Rodekamp made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Fix Version/s Release Branch 10.04 [ 12314832 ]
        Resolution Fixed [ 1 ]
        Hide
        Leon added a comment - - edited

        it solves the "DVD+R" problem, but seems not solve ""Bäume" problem, anything I missed?

        --yeah, I missed the patch to CatalinaContainer.java. Thanks Wojciech Szymanowski.

        Show
        Leon added a comment - - edited it solves the "DVD+R" problem, but seems not solve ""Bäume" problem, anything I missed? --yeah, I missed the patch to CatalinaContainer.java. Thanks Wojciech Szymanowski.

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to https://fisheye6.atlassian.com/ failed: Error in remote call to 'FishEye 0 (https://fisheye6.atlassian.com/)' (https://fisheye6.atlassian.com) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=OFBIZ-2628, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Failed to parse FishEye response: Error on line 23 of document : An invalid XML character (Unicode: 0x1a) was found in the element content of the document. Nested exception: An invalid XML character (Unicode: 0x1a) was found in the element content of the document.

          People

          • Assignee:
            Sascha Rodekamp
            Reporter:
            Patrick Antivackis
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 4h
              4h
              Remaining:
              Remaining Estimate - 4h
              4h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development