Re: svn commit: r772699 - in /ofbiz/trunk/framework: base/src/org/ofbiz/base/util/ base/src/org/ofbiz/base/util/string/ widget/src/org/ofbiz/widget/form/

[Adam Heath-2]

[hidden email] wrote:

> Author: adrianc
> Date: Thu May  7 16:30:02 2009
> New Revision: 772699
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java?rev=772699&r1=772698&r2=772699&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java Thu May  7 16:30:02 2009
> @@ -62,19 +61,17 @@
>              Debug.logError("BSH Evaluation error. Empty expression", module);
>              return null;
>          }
> -
> -        if (Debug.verboseOn())
> +        if (Debug.verboseOn()) {
>              Debug.logVerbose("Evaluating -- " + expression, module);
> -        if (Debug.verboseOn())
>              Debug.logVerbose("Using Context -- " + context, module);
> -
> +        }
>          try {
>              Interpreter bsh = makeInterpreter(context);
>              // evaluate the expression
> -            o = bsh.eval(expression);
> -            if (Debug.verboseOn())
> +            o = bsh.eval(StringUtil.convertOperatorSubstitutions(expression));
> +            if (Debug.verboseOn()) {
>                  Debug.logVerbose("Evaluated to -- " + o, module);
> -
> +            }
>              // read back the context info
>              NameSpace ns = bsh.getNameSpace();
>              String[] varNames = ns.getVariableNames();

Please try not do do whitespace/formatting changes when altering code
paths.

> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java?rev=772699&r1=772698&r2=772699&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java Thu May  7 16:30:02 2009
> @@ -56,6 +56,7 @@
>  public class StringUtil {
>  
>      public static final String module = StringUtil.class.getName();
> +    protected static final Map<String, Pattern> substitionPatternMap;

substitution, you mispelled(sic) the word.

Additionally, this is not really a map.  It's more a list of
pattern/replacement pairs.

>      /** OWASP ESAPI canonicalize strict flag; setting false so we only get warnings about double encoding, etc; can be set to true for exceptions and more security */
>      public static final boolean esapiCanonicalizeStrict = false;
> @@ -66,6 +67,14 @@
>          List<Codec> codecList = Arrays.asList(new HTMLEntityCodec(), new PercentCodec());
>          defaultWebEncoder = new DefaultEncoder(codecList);
>          defaultWebValidator = new DefaultValidator();
> +        substitionPatternMap = FastMap.newInstance();
> +        substitionPatternMap.put("&&", Pattern.compile("@and", Pattern.LITERAL));
> +        substitionPatternMap.put("||", Pattern.compile("@or", Pattern.LITERAL));
> +        substitionPatternMap.put("<=", Pattern.compile("@lteq", Pattern.LITERAL));
> +        substitionPatternMap.put(">=", Pattern.compile("@gteq", Pattern.LITERAL));
> +        substitionPatternMap.put("<", Pattern.compile("@lt", Pattern.LITERAL));
> +        substitionPatternMap.put(">", Pattern.compile("@gt", Pattern.LITERAL));
> +        substitionPatternMap.put("\"", Pattern.compile("'", Pattern.LITERAL));
>      }
>  
>      public static final SimpleEncoder htmlEncoder = new HtmlEncoder();
> @@ -474,6 +483,33 @@
>          return outStrBfr.toString();
>      }
>  
> +    public static String convertOperatorSubstitutions(String expression) {
> +        String result = expression;
> +        if (result != null && result.contains("@")) {
> +            Set<String> keys = substitionPatternMap.keySet();
> +            for (String replacement : keys) {

for (Map.Entry<String, Pattern> entry: substitionPatternMap) {

Don't do a loop, then a separate fetch.  The above is more efficient.

> +                Pattern pattern = substitionPatternMap.get(replacement);
> +                result = pattern.matcher(result).replaceAll(replacement);

There is probably a more efficient way to do this, instead of looping
over the entire string for each listing pattern/replacement.  Maybe
combining all the patterns in a (foo|bar) arrangement, looping with
appendReplacement, then looking up the matched value in the key.  But
I could be wrong on that.

> +            }
> +        }
> +        return result;
> +    }
> +
>      /**
>       * Uses a black-list approach for necessary characters for HTML.
>       * Does not allow various characters (after canonicalization), including "<", ">", "&" (if not followed by a space), and "%" (if not followed by a space).
>

[Adrian Crum]

Adam,

Thank you for your review and comments. There might be a more efficient
way to do this, but this is the best I can come up with. It's an
improvement over the repeated String replace alls.

-Adrian


Adam Heath wrote:

> [hidden email] wrote:
>> Author: adrianc
>> Date: Thu May  7 16:30:02 2009
>> New Revision: 772699
>>
>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java?rev=772699&r1=772698&r2=772699&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java (original)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/BshUtil.java Thu May  7 16:30:02 2009
>> @@ -62,19 +61,17 @@
>>              Debug.logError("BSH Evaluation error. Empty expression", module);
>>              return null;
>>          }
>> -
>> -        if (Debug.verboseOn())
>> +        if (Debug.verboseOn()) {
>>              Debug.logVerbose("Evaluating -- " + expression, module);
>> -        if (Debug.verboseOn())
>>              Debug.logVerbose("Using Context -- " + context, module);
>> -
>> +        }
>>          try {
>>              Interpreter bsh = makeInterpreter(context);
>>              // evaluate the expression
>> -            o = bsh.eval(expression);
>> -            if (Debug.verboseOn())
>> +            o = bsh.eval(StringUtil.convertOperatorSubstitutions(expression));
>> +            if (Debug.verboseOn()) {
>>                  Debug.logVerbose("Evaluated to -- " + o, module);
>> -
>> +            }
>>              // read back the context info
>>              NameSpace ns = bsh.getNameSpace();
>>              String[] varNames = ns.getVariableNames();
>
> Please try not do do whitespace/formatting changes when altering code
> paths.
>
>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java?rev=772699&r1=772698&r2=772699&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java (original)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/StringUtil.java Thu May  7 16:30:02 2009
>> @@ -56,6 +56,7 @@
>>  public class StringUtil {
>>  
>>      public static final String module = StringUtil.class.getName();
>> +    protected static final Map<String, Pattern> substitionPatternMap;
>
> substitution, you mispelled(sic) the word.
>
> Additionally, this is not really a map.  It's more a list of
> pattern/replacement pairs.
>
>>      /** OWASP ESAPI canonicalize strict flag; setting false so we only get warnings about double encoding, etc; can be set to true for exceptions and more security */
>>      public static final boolean esapiCanonicalizeStrict = false;
>> @@ -66,6 +67,14 @@
>>          List<Codec> codecList = Arrays.asList(new HTMLEntityCodec(), new PercentCodec());
>>          defaultWebEncoder = new DefaultEncoder(codecList);
>>          defaultWebValidator = new DefaultValidator();
>> +        substitionPatternMap = FastMap.newInstance();
>> +        substitionPatternMap.put("&&", Pattern.compile("@and", Pattern.LITERAL));
>> +        substitionPatternMap.put("||", Pattern.compile("@or", Pattern.LITERAL));
>> +        substitionPatternMap.put("<=", Pattern.compile("@lteq", Pattern.LITERAL));
>> +        substitionPatternMap.put(">=", Pattern.compile("@gteq", Pattern.LITERAL));
>> +        substitionPatternMap.put("<", Pattern.compile("@lt", Pattern.LITERAL));
>> +        substitionPatternMap.put(">", Pattern.compile("@gt", Pattern.LITERAL));
>> +        substitionPatternMap.put("\"", Pattern.compile("'", Pattern.LITERAL));
>>      }
>>  
>>      public static final SimpleEncoder htmlEncoder = new HtmlEncoder();
>> @@ -474,6 +483,33 @@
>>          return outStrBfr.toString();
>>      }
>>  
>> +    public static String convertOperatorSubstitutions(String expression) {
>> +        String result = expression;
>> +        if (result != null && result.contains("@")) {
>> +            Set<String> keys = substitionPatternMap.keySet();
>> +            for (String replacement : keys) {
>
> for (Map.Entry<String, Pattern> entry: substitionPatternMap) {
>
> Don't do a loop, then a separate fetch.  The above is more efficient.
>
>> +                Pattern pattern = substitionPatternMap.get(replacement);
>> +                result = pattern.matcher(result).replaceAll(replacement);
>
> There is probably a more efficient way to do this, instead of looping
> over the entire string for each listing pattern/replacement.  Maybe
> combining all the patterns in a (foo|bar) arrangement, looping with
> appendReplacement, then looking up the matched value in the key.  But
> I could be wrong on that.
>
>> +            }
>> +        }
>> +        return result;
>> +    }
>> +
>>      /**
>>       * Uses a black-list approach for necessary characters for HTML.
>>       * Does not allow various characters (after canonicalization), including "<", ">", "&" (if not followed by a space), and "%" (if not followed by a space).
>>
>
>