You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2019/11/09 13:32:41 UTC
[ofbiz-framework] branch trunk updated: Fixed: (OFBIZ-)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 5170e9d Fixed: (OFBIZ-)
5170e9d is described below
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
@@ -20,6 +20,7 @@ package org.apache.ofbiz.product.category;
import java.io.IOException;
import java.net.URL;
+import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
@@ -41,6 +42,8 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
+import org.apache.http.client.utils.URLEncodedUtils;
+import org.apache.http.message.BasicNameValuePair;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.StringUtil;
import org.apache.ofbiz.base.util.UtilHttp;
@@ -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) {
+ List<BasicNameValuePair> params = new ArrayList<BasicNameValuePair>();
+ 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;
+ }
+
boolean forwarded = forwardUri(httpResponse, uri);
if (forwarded) {
return;
@@ -218,7 +234,7 @@ public class SeoContextFilter implements Filter {
* @param uri String to reverse transform
* @return String
*/
- protected static boolean forwardUri(HttpServletResponse response, String uri) {
+ private static boolean forwardUri(HttpServletResponse response, String uri) {
Perl5Matcher matcher = new Perl5Matcher();
boolean foundMatch = false;
Integer responseCodeInt = null;
Re: [ofbiz-framework] branch trunk updated: Fixed: (OFBIZ-)
Posted by Jacques Le Roux <ja...@les7arts.com>.
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---
>
> jleroux@apache.org 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
Re: [ofbiz-framework] branch trunk updated: Fixed: (OFBIZ-)
Posted by Mathieu Lirzin <ma...@nereide.fr>.
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---
jleroux@apache.org 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
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
Re: [ofbiz-framework] branch trunk updated: Fixed: (OFBIZ-)
Posted by Mathieu Lirzin <ma...@nereide.fr>.
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