You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Mathieu Lirzin <ma...@nereide.fr> on 2019/11/09 14:17:00 UTC
Re: [ofbiz-framework] branch trunk updated: Fixed: (OFBIZ-)
Hello Jacques,
jleroux@apache.org writes:
> commit 5170e9d89503afa13d4ea1492b0ba73b2f92e528
> Author: Jacques Le Roux <ja...@les7arts.com>
> AuthorDate: Sat Nov 9 14:25:00 2019 +0100
>
> Fixed:
> (OFBIZ-)
>
> While working on OFBIZ-9804 (verification email for Newsletter) I was confronted
> with misc. issues. One of them was that SeoContextFilter.java was not handling
> query strings.
>
> This fixes it
> ---
> .../ofbiz/product/category/SeoContextFilter.java | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java b/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java
> index 17ab0ae..b7cab04 100644
> --- a/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java
> +++ b/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java
> @@ -96,6 +99,19 @@ public class SeoContextFilter implements Filter {
> HttpServletResponse httpResponse = (HttpServletResponse) response;
>
> String uri = httpRequest.getRequestURI();
> +
> + Map<String, String[]> parameterMap =request.getParameterMap();
> + if (parameterMap != null) {
The documentation of ‘getParameterMap’ is not stating that the return
value can be ‘null’ contrary to what is done in other methods of the
same class so I guess we could assume that an empty map is returned.
> + List<BasicNameValuePair> params = new ArrayList<BasicNameValuePair>();
^^^
Unnecessary type declaration
> + request.getParameterMap().forEach((name, values) -> {
> + for(String value : values) {
> + params.add(new BasicNameValuePair(name, value));
> + }
> + });
> + String queryString = URLEncodedUtils.format(params, Charset.forName("UTF-8"));
> + uri = uri + "?" + queryString;
> + }
Is there any reason why you are using ‘URLEncodedUtils.format’ instead
of simply retrieving the query string from the servlet request, like
this?
--8<---------------cut here---------------start------------->8---
String uri = httpRequest.getRequestURI();
String queryString = httpRequest.getQueryString();
if (queryString != null) {
uri = uri + "?" + queryString;
}
boolean forwarded = forwardUri(httpResponse, uri);
--8<---------------cut here---------------end--------------->8---
In order to help understand what is the expected behavior of
‘SeoContextFilter#doFilter’ and make the bug fix more future proof, it
would be really nice if you could add a unit test demonstrating how
query string examples should be handled by this fiter.
Thanks.
--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761 070D 0ADE E100 9460 4D37
Re: [ofbiz-framework] branch trunk updated: Fixed: (OFBIZ-)
Posted by Jacques Le Roux <ja...@les7arts.com>.
Le 09/11/2019 à 15:17, Mathieu Lirzin a écrit :
> Hello Jacques,
>
> jleroux@apache.org writes:
>
>> +++ b/applications/product/src/main/java/org/apache/ofbiz/product/category/SeoContextFilter.java
>> @@ -96,6 +99,19 @@ public class SeoContextFilter implements Filter {
>> HttpServletResponse httpResponse = (HttpServletResponse) response;
>>
>> String uri = httpRequest.getRequestURI();
>> +
>> + Map<String, String[]> parameterMap =request.getParameterMap();
>> + if (parameterMap != null) {
> The documentation of ‘getParameterMap’ is not stating that the return
> value can be ‘null’ contrary to what is done in other methods of the
> same class so I guess we could assume that an empty map is returned.
Good point, I'll amend that
> + List<BasicNameValuePair> params = new ArrayList<BasicNameValuePair>();
> ^^^
> Unnecessary type declaration
>> + request.getParameterMap().forEach((name, values) -> {
>> + for(String value : values) {
>> + params.add(new BasicNameValuePair(name, value));
>> + }
>> + });
>> + String queryString = URLEncodedUtils.format(params, Charset.forName("UTF-8"));
>> + uri = uri + "?" + queryString;
>> + }
>
> Is there any reason why you are using ‘URLEncodedUtils.format’ instead
> of simply retrieving the query string from the servlet request, like
> this?
>
> --8<---------------cut here---------------start------------->8---
> String uri = httpRequest.getRequestURI();
> String queryString = httpRequest.getQueryString();
> if (queryString != null) {
> uri = uri + "?" + queryString;
> }
> boolean forwarded = forwardUri(httpResponse, uri);
> --8<---------------cut here---------------end--------------->8---
Yes, in the case I was struggling with there is no query string, it's only hidden parameters.
In theory I could try 1st if a query string exists. Unfortunately so far there are no such cases.
Actually if there was a query string I'd not need to add this code block.
> In order to help understand what is the expected behavior of
> ‘SeoContextFilter#doFilter’ and make the bug fix more future proof, it
> would be really nice if you could add a unit test demonstrating how
> query string examples should be handled by this fiter.
>
> Thanks.
OK, I'll try that after finishing the stuff I was initially working on, ie OFBIZ-9804.
Please remind me if I forget ;)
Jacques