Re: [ofbiz-framework] branch trunk updated: Fixed: (OFBIZ-)

[Mathieu Lirzin]

Hello Jacques,

[hidden email] writes:

> commit 5170e9d89503afa13d4ea1492b0ba73b2f92e528
> Author: Jacques Le Roux <[hidden email]>
> 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

[Mathieu Lirzin]

Hello again,

‘./gradlew check’ fails after this commit, because it introduces some
linting issues.

> Task :checkstyleMain FAILED

--8<---------------cut here---------------start------------->8---
FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: file:///home/mthl/src/ofbiz/build/reports/checkstyle/main.html
  Checkstyle files with violations: 1039
  Checkstyle violations by severity: [error:37783]
--8<---------------cut here---------------end--------------->8---

[hidden email] writes:

[...]
> +
> +        Map<String, String[]> parameterMap =request.getParameterMap();
                                               ^ missing space
> +        if (parameterMap != null) {
> +            List<BasicNameValuePair> params = new ArrayList<BasicNameValuePair>();
> +            request.getParameterMap().forEach((name, values) -> {
> +                for(String value : values) {
                      ^ missing space
> +                    params.add(new BasicNameValuePair(name, value));
> +                }
> +            });
> +            String queryString = URLEncodedUtils.format(params, Charset.forName("UTF-8"));
> +            uri = uri + "?" + queryString;
                                             ^ trailing whitespace
> +        }
> +        
   ^^^^^^^^^trailing whitespaces

Can you fix those ? :-)

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Jacques Le Roux]

Le 09/11/2019 à 15:17, Mathieu Lirzin a écrit :

> Hello Jacques,
>
> [hidden email] 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

[Jacques Le Roux]

Le 09/11/2019 à 15:31, Mathieu Lirzin a écrit :

> Hello again,
>
> ‘./gradlew check’ fails after this commit, because it introduces some
> linting issues.
>
>> Task :checkstyleMain FAILED
> --8<---------------cut here---------------start------------->8---
> FAILURE: Build failed with an exception.
>
> * What went wrong:
> Execution failed for task ':checkstyleMain'.
>> Checkstyle rule violations were found. See the report at: file:///home/mthl/src/ofbiz/build/reports/checkstyle/main.html
>    Checkstyle files with violations: 1039
>    Checkstyle violations by severity: [error:37783]
> --8<---------------cut here---------------end--------------->8---
>
> [hidden email] writes:
>
> [...]
>> +
>> +        Map<String, String[]> parameterMap =request.getParameterMap();
>                                                 ^ missing space
>> +        if (parameterMap != null) {
>> +            List<BasicNameValuePair> params = new ArrayList<BasicNameValuePair>();
>> +            request.getParameterMap().forEach((name, values) -> {
>> +                for(String value : values) {
>                        ^ missing space
>> +                    params.add(new BasicNameValuePair(name, value));
>> +                }
>> +            });
>> +            String queryString = URLEncodedUtils.format(params, Charset.forName("UTF-8"));
>> +            uri = uri + "?" + queryString;
>                                               ^ trailing whitespace
>> +        }
>> +
>     ^^^^^^^^^trailing whitespaces
>
> Can you fix those ? :-)

Yes, sure. I'll do with the parameterMap empty, not null, check. I thought I did, certainly "some things happened" before the commit

Jacques