You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by ad...@apache.org on 2010/02/02 17:35:33 UTC
svn commit: r905696 -
/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java
Author: adrianc
Date: Tue Feb 2 16:35:28 2010
New Revision: 905696
URL: http://svn.apache.org/viewvc?rev=905696&view=rev
Log:
Made a small change to FlexibleStringExpander - created a NullElem class to support existing client code that tested getOriginal() for null. By the way, that is a bad practice - use the isEmpty() method instead.
Problem reported by Scott and Hans on the dev mailing list.
Modified:
ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java
Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java
URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java?rev=905696&r1=905695&r2=905696&view=diff
==============================================================================
--- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java (original)
+++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java Tue Feb 2 16:35:28 2010
@@ -56,7 +56,7 @@
public static final String openBracket = "${";
public static final String closeBracket = "}";
protected static final UtilCache<String, FlexibleStringExpander> exprCache = UtilCache.createUtilCache("flexibleStringExpander.ExpressionCache");
- protected static final FlexibleStringExpander nullExpr = new ConstElem("");
+ protected static final FlexibleStringExpander nullExpr = new NullElem();
/** Does on-the-fly parsing and expansion of the original String using
* variable values from the passed context. A null context argument will
@@ -400,6 +400,26 @@
}
}
+ protected static class NullElem extends FlexibleStringExpander {
+ protected NullElem() {
+ super("");
+ }
+ @Override
+ public void append(StringBuilder buffer, Map<String, ? extends Object> context, TimeZone timeZone, Locale locale) {}
+ @Override
+ public String expandString(Map<String, ? extends Object> context, TimeZone timeZone, Locale locale) {
+ return null;
+ }
+ @Override
+ public String getOriginal() {
+ return null;
+ }
+ @Override
+ public boolean isEmpty() {
+ return true;
+ }
+ }
+
protected static class VarElem extends FlexibleStringExpander {
protected final char[] bracketedOriginal;
protected VarElem(String original) {
Re: svn commit: r905696 -
/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java
Posted by David E Jones <de...@me.com>.
On Feb 2, 2010, at 10:47 AM, Adam Heath wrote:
> adrianc@apache.org wrote:
>> Author: adrianc
>> Date: Tue Feb 2 16:35:28 2010
>> New Revision: 905696
>>
>> URL: http://svn.apache.org/viewvc?rev=905696&view=rev
>> Log:
>> Made a small change to FlexibleStringExpander - created a NullElem class to support existing client code that tested getOriginal() for null. By the way, that is a bad practice - use the isEmpty() method instead.
>>
>> Problem reported by Scott and Hans on the dev mailing list.
>
> Test case?
>
> You should write a bunch of test cases against FSE, being certain to
> not use any internal methods or classes. Then, revert back to older
> FSE, and run the same test suite, to see if it still functions.
>
> I did that once with our COW filesystem. It was running too slow, and
> I couldn't find any obvious flaws. I then rewrote it(without test
> cases), giving it a new name and new classes. The site access went
> from 13req/s to 500+req/s when I did this. However, I wasn't
> comfortable switching all deployed things to it yet.
>
> So, I wrote a single test suite, that could test either version, and
> used cobertura to ensure that I was actually accessing most lines and
> branches in both classes. Then I felt comfortable switching
> everything in mass.
>
> I think that a proper FSE-only test suite would have discovered this
> in the first place. If the old version allowed getOriginal() to
> return null, then the new version would have failed the test.
It might have, but only if the author of the test suite anticipated this issue. If the author was not aware of this issue or able to predict the future in this way, then it wouldn't do ANY good whatsoever, except perhaps make people more confident that there are no issues, which may lead people away from finding the actual issue... ;)
-David
Re: svn commit: r905696 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/string/FlexibleStringExpander.java
Posted by Adam Heath <do...@brainfood.com>.
adrianc@apache.org wrote:
> Author: adrianc
> Date: Tue Feb 2 16:35:28 2010
> New Revision: 905696
>
> URL: http://svn.apache.org/viewvc?rev=905696&view=rev
> Log:
> Made a small change to FlexibleStringExpander - created a NullElem class to support existing client code that tested getOriginal() for null. By the way, that is a bad practice - use the isEmpty() method instead.
>
> Problem reported by Scott and Hans on the dev mailing list.
Test case?
You should write a bunch of test cases against FSE, being certain to
not use any internal methods or classes. Then, revert back to older
FSE, and run the same test suite, to see if it still functions.
I did that once with our COW filesystem. It was running too slow, and
I couldn't find any obvious flaws. I then rewrote it(without test
cases), giving it a new name and new classes. The site access went
from 13req/s to 500+req/s when I did this. However, I wasn't
comfortable switching all deployed things to it yet.
So, I wrote a single test suite, that could test either version, and
used cobertura to ensure that I was actually accessing most lines and
branches in both classes. Then I felt comfortable switching
everything in mass.
I think that a proper FSE-only test suite would have discovered this
in the first place. If the old version allowed getOriginal() to
return null, then the new version would have failed the test.