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 2013/11/13 19:04:14 UTC
svn commit: r1541641 - in /ofbiz/trunk:
applications/party/src/org/ofbiz/party/communication/
applications/product/src/org/ofbiz/product/product/
framework/common/src/org/ofbiz/common/login/
framework/webapp/src/org/ofbiz/webapp/control/
Author: jleroux
Date: Wed Nov 13 18:04:14 2013
New Revision: 1541641
URL: http://svn.apache.org/r1541641
Log:
java.util.regex.Pattern instances are immutable but not instances of the Matcher companion class.
So better be safe than sorry, even if in the loops we loose a bit of performance (here only "short" loops anyway)
Modified:
ofbiz/trunk/applications/party/src/org/ofbiz/party/communication/CommunicationEventServices.java
ofbiz/trunk/applications/product/src/org/ofbiz/product/product/ProductEvents.java
ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java
ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
Modified: ofbiz/trunk/applications/party/src/org/ofbiz/party/communication/CommunicationEventServices.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/src/org/ofbiz/party/communication/CommunicationEventServices.java?rev=1541641&r1=1541640&r2=1541641&view=diff
==============================================================================
--- ofbiz/trunk/applications/party/src/org/ofbiz/party/communication/CommunicationEventServices.java (original)
+++ ofbiz/trunk/applications/party/src/org/ofbiz/party/communication/CommunicationEventServices.java Wed Nov 13 18:04:14 2013
@@ -18,8 +18,6 @@
*******************************************************************************/
package org.ofbiz.party.communication;
-import org.ofbiz.base.util.GeneralException;
-
import java.io.IOException;
import java.io.InputStream;
import java.net.MalformedURLException;
@@ -32,7 +30,6 @@ import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
-import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.mail.Address;
@@ -47,6 +44,7 @@ import javolution.util.FastMap;
import org.ofbiz.base.location.FlexibleLocation;
import org.ofbiz.base.util.Debug;
+import org.ofbiz.base.util.GeneralException;
import org.ofbiz.base.util.StringUtil;
import org.ofbiz.base.util.UtilDateTime;
import org.ofbiz.base.util.UtilHttp;
@@ -1187,10 +1185,9 @@ public class CommunicationEventServices
// find the "Action" element and obtain its value (looking for "failed")
Pattern p2 = Pattern.compile("^Action: (.*)$", Pattern.MULTILINE | Pattern.CASE_INSENSITIVE);
- Matcher m2 = p2.matcher(part2Text);
String action = null;
- if (m2.find()) {
- action = m2.group(1);
+ if (p2.matcher(part2Text).find()) {
+ action = p2.matcher(part2Text).group(1);
}
if (action != null && "failed".equalsIgnoreCase(action)) {
@@ -1204,11 +1201,10 @@ public class CommunicationEventServices
// find the "Message-Id" element and obtain its value (looking for "failed")
Pattern p3 = Pattern.compile("^Message-Id: (.*)$", Pattern.MULTILINE | Pattern.CASE_INSENSITIVE);
- Matcher m3 = p3.matcher(part3Text);
String messageId = null;
- if (m3.find()) {
- Debug.logInfo("Found message-id : " + m3.group(), module);
- messageId = m3.group(1);
+ if (p3.matcher(part3Text).find()) {
+ Debug.logInfo("Found message-id : " + p3.matcher(part3Text).group(), module);
+ messageId = p3.matcher(part3Text).group(1);
}
// find the matching communication event
Modified: ofbiz/trunk/applications/product/src/org/ofbiz/product/product/ProductEvents.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/product/src/org/ofbiz/product/product/ProductEvents.java?rev=1541641&r1=1541640&r2=1541641&view=diff
==============================================================================
--- ofbiz/trunk/applications/product/src/org/ofbiz/product/product/ProductEvents.java (original)
+++ ofbiz/trunk/applications/product/src/org/ofbiz/product/product/ProductEvents.java Wed Nov 13 18:04:14 2013
@@ -24,7 +24,6 @@ import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
-import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.servlet.http.HttpServletRequest;
@@ -1187,9 +1186,8 @@ public class ProductEvents {
if (UtilValidate.isNotEmpty(productId) && UtilValidate.isNotEmpty(productTags)) {
List<String> matchList = FastList.newInstance();
Pattern regex = Pattern.compile("[^\\s\"']+|\"([^\"]*)\"|'([^']*)'");
- Matcher regexMatcher = regex.matcher(productTags);
- while (regexMatcher.find()) {
- matchList.add(regexMatcher.group().replace("'", ""));
+ while (regex.matcher(productTags).find()) {
+ matchList.add(regex.matcher(productTags).group().replace("'", ""));
}
GenericValue userLogin = null;
Modified: ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java?rev=1541641&r1=1541640&r2=1541641&view=diff
==============================================================================
--- ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java (original)
+++ ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java Wed Nov 13 18:04:14 2013
@@ -23,7 +23,6 @@ import java.sql.Timestamp;
import java.util.List;
import java.util.Locale;
import java.util.Map;
-import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.transaction.Transaction;
@@ -957,9 +956,7 @@ public class LoginServices {
boolean usePasswordPattern = UtilProperties.getPropertyAsBoolean("security.properties", "security.login.password.pattern.enable", true);
if (usePasswordPattern) {
Pattern pattern = Pattern.compile(passwordPattern);
- Matcher matcher = pattern.matcher(newPassword);
- boolean matched = matcher.matches();
- if (!matched) {
+ if (!pattern.matcher(newPassword).matches()) {
// This is a mix to handle the OOTB pattern which is only a fixed length
Map<String, String> messageMap = UtilMisc.toMap("minPasswordLength", Integer.toString(minPasswordLength));
String passwordPatternMessage = UtilProperties.getPropertyValue("security.properties",
Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1541641&r1=1541640&r2=1541641&view=diff
==============================================================================
--- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original)
+++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Wed Nov 13 18:04:14 2013
@@ -26,7 +26,6 @@ import java.sql.Timestamp;
import java.util.List;
import java.util.Map;
import java.util.ServiceLoader;
-import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.servlet.ServletContext;
@@ -925,9 +924,8 @@ public class LoginWorker {
if (i == 0) {
String cn = x500Map.get("CN");
cn = cn.replaceAll("\\\\", "");
- Matcher m = pattern.matcher(cn);
- if (m.matches()) {
- userLoginId = m.group(1);
+ if (pattern.matcher(cn).matches()) {
+ userLoginId = pattern.matcher(cn).group(1);
} else {
Debug.logInfo("Client certificate CN does not match pattern: [" + cnPattern + "]", module);
}
Re: svn commit: r1541641 - in /ofbiz/trunk:
applications/party/src/org/ofbiz/party/communication/
applications/product/src/org/ofbiz/product/product/
framework/common/src/org/ofbiz/common/login/
framework/webapp/src/org/ofbiz/webapp/control/
Posted by Jacques Le Roux <ja...@les7arts.com>.
It depends on what required means here. I committed those because Matcher class is not thread safe.
I must say I did not cross any issues before doing these changes. They are more thread safe issues prevention.
So I believe it's safer to do it this way. Though I'm not quite sure pattern.matcher is atomic. So maybe using synchronized could be required to guarantee thread safe.
But contrary to the change I did in ProductEvents.java and changed again at r1541746, synchronized is not necessary here (nor in other changes I did in r1541641)
It's the same behaviour (compiled pattern is not reused) than before whith a chance that pattern.matcher is atomic, thus prevents possible issues.
What are your concerns?
Jacques
On Thursday, November 14, 2013 10:25 AM Jacopo Cappellato <ja...@hotwaxmedia.com> wrote:
> Are these changes required? Can we revert them?
>
> Jacopo
>
>
> On Nov 13, 2013, at 7:04 PM, jleroux@apache.org wrote:
>
>> Modified: ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java?rev=1541641&r1=1541640&r2=1541641&view=diff
>> ============================================================================== ---
>> ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java (original) +++
>> ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java Wed Nov 13 18:04:14 2013 @@ -23,7 +23,6 @@ import
>> java.sql.Timestamp;
>> import java.util.List;
>> import java.util.Locale;
>> import java.util.Map;
>> -import java.util.regex.Matcher;
>> import java.util.regex.Pattern;
>>
>> import javax.transaction.Transaction;
>> @@ -957,9 +956,7 @@ public class LoginServices {
>> boolean usePasswordPattern = UtilProperties.getPropertyAsBoolean("security.properties",
>> "security.login.password.pattern.enable", true); if (usePasswordPattern) {
>> Pattern pattern = Pattern.compile(passwordPattern);
>> - Matcher matcher = pattern.matcher(newPassword);
>> - boolean matched = matcher.matches();
>> - if (!matched) {
>> + if (!pattern.matcher(newPassword).matches()) {
>> // This is a mix to handle the OOTB pattern which is only a fixed length
>> Map<String, String> messageMap = UtilMisc.toMap("minPasswordLength", Integer.toString(minPasswordLength));
>> String passwordPatternMessage = UtilProperties.getPropertyValue("security.properties",
>>
>> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1541641&r1=1541640&r2=1541641&view=diff
>> ============================================================================== ---
>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original) +++
>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Wed Nov 13 18:04:14 2013 @@ -26,7 +26,6 @@ import
>> java.sql.Timestamp;
>> import java.util.List;
>> import java.util.Map;
>> import java.util.ServiceLoader;
>> -import java.util.regex.Matcher;
>> import java.util.regex.Pattern;
>>
>> import javax.servlet.ServletContext;
>> @@ -925,9 +924,8 @@ public class LoginWorker {
>> if (i == 0) {
>> String cn = x500Map.get("CN");
>> cn = cn.replaceAll("\\\\", "");
>> - Matcher m = pattern.matcher(cn);
>> - if (m.matches()) {
>> - userLoginId = m.group(1);
>> + if (pattern.matcher(cn).matches()) {
>> + userLoginId = pattern.matcher(cn).group(1);
>> } else {
>> Debug.logInfo("Client certificate CN does not match pattern: [" + cnPattern + "]", module);
>> }
Re: svn commit: r1541641 - in /ofbiz/trunk: applications/party/src/org/ofbiz/party/communication/ applications/product/src/org/ofbiz/product/product/ framework/common/src/org/ofbiz/common/login/ framework/webapp/src/org/ofbiz/webapp/control/
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
Are these changes required? Can we revert them?
Jacopo
On Nov 13, 2013, at 7:04 PM, jleroux@apache.org wrote:
> Modified: ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java?rev=1541641&r1=1541640&r2=1541641&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java (original)
> +++ ofbiz/trunk/framework/common/src/org/ofbiz/common/login/LoginServices.java Wed Nov 13 18:04:14 2013
> @@ -23,7 +23,6 @@ import java.sql.Timestamp;
> import java.util.List;
> import java.util.Locale;
> import java.util.Map;
> -import java.util.regex.Matcher;
> import java.util.regex.Pattern;
>
> import javax.transaction.Transaction;
> @@ -957,9 +956,7 @@ public class LoginServices {
> boolean usePasswordPattern = UtilProperties.getPropertyAsBoolean("security.properties", "security.login.password.pattern.enable", true);
> if (usePasswordPattern) {
> Pattern pattern = Pattern.compile(passwordPattern);
> - Matcher matcher = pattern.matcher(newPassword);
> - boolean matched = matcher.matches();
> - if (!matched) {
> + if (!pattern.matcher(newPassword).matches()) {
> // This is a mix to handle the OOTB pattern which is only a fixed length
> Map<String, String> messageMap = UtilMisc.toMap("minPasswordLength", Integer.toString(minPasswordLength));
> String passwordPatternMessage = UtilProperties.getPropertyValue("security.properties",
>
> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java?rev=1541641&r1=1541640&r2=1541641&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java (original)
> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/LoginWorker.java Wed Nov 13 18:04:14 2013
> @@ -26,7 +26,6 @@ import java.sql.Timestamp;
> import java.util.List;
> import java.util.Map;
> import java.util.ServiceLoader;
> -import java.util.regex.Matcher;
> import java.util.regex.Pattern;
>
> import javax.servlet.ServletContext;
> @@ -925,9 +924,8 @@ public class LoginWorker {
> if (i == 0) {
> String cn = x500Map.get("CN");
> cn = cn.replaceAll("\\\\", "");
> - Matcher m = pattern.matcher(cn);
> - if (m.matches()) {
> - userLoginId = m.group(1);
> + if (pattern.matcher(cn).matches()) {
> + userLoginId = pattern.matcher(cn).group(1);
> } else {
> Debug.logInfo("Client certificate CN does not match pattern: [" + cnPattern + "]", module);
> }