Repository: incubator-freemarker Updated Branches: refs/heads/2.3-gae fca65a24f -> eef30af2f
Bug fixed: It wasn't well defined when a Java Iterator counts as empty. Depending on what ObjectWrapper you are using, one of these fixes apply: (a) DefaultObjectWrapper (fix is always active): Operations on the Iterator that only check if it's empty without reading an element from it, such as ?has_content, won't cause the a later iteration (or further emptiness check) to fail anymore. Earlier, in certain situations, the second operation has failed saying that the iterator can be listed only once. (b) BeansWrapper (when it's not extended by DefaultObjectWrapper), if it's incompatibleImprovements property is set to 2.3.24 (or higher): Iterator-s were always said to be non-empty when using ?has_content and such (i.e., operators that check emptiness without reading any elements). Now an Iterator counts as empty exactly if it has no elements left. (Note that this bug has never affected basic functionality, like <#list ...>.) Project: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/commit/eef30af2 Tree: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/tree/eef30af2 Diff: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/diff/eef30af2 Branch: refs/heads/2.3-gae Commit: eef30af2fa1b61be9ad553f92362e5ff72a15779 Parents: fca65a2 Author: ddekany <[email protected]> Authored: Sun Oct 25 00:00:14 2015 +0200 Committer: ddekany <[email protected]> Committed: Sun Oct 25 00:02:24 2015 +0200 ---------------------------------------------------------------------- .../java/freemarker/ext/beans/BeanModel.java | 4 + .../java/freemarker/ext/beans/BeansWrapper.java | 19 ++- .../java/freemarker/template/Configuration.java | 11 ++ .../template/DefaultIteratorAdapter.java | 15 +- .../template/DefaultObjectWrapper.java | 5 + .../freemarker/template/SimpleCollection.java | 24 +-- src/manual/book.xml | 34 +++++ .../freemarker/core/IteratorIssuesTest.java | 149 +++++++++++++++++++ .../template/DefaultObjectWrapperTest.java | 2 +- 9 files changed, 242 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/main/java/freemarker/ext/beans/BeanModel.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/ext/beans/BeanModel.java b/src/main/java/freemarker/ext/beans/BeanModel.java index 26eb1f5..794a66b 100644 --- a/src/main/java/freemarker/ext/beans/BeanModel.java +++ b/src/main/java/freemarker/ext/beans/BeanModel.java @@ -27,6 +27,7 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -296,6 +297,9 @@ implements if (object instanceof Collection) { return ((Collection) object).isEmpty(); } + if (object instanceof Iterator && wrapper.is2324Bugfixed()) { + return !((Iterator) object).hasNext(); + } if (object instanceof Map) { return ((Map) object).isEmpty(); } http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/main/java/freemarker/ext/beans/BeansWrapper.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/ext/beans/BeansWrapper.java b/src/main/java/freemarker/ext/beans/BeansWrapper.java index b21bdeb..c809fe7 100644 --- a/src/main/java/freemarker/ext/beans/BeansWrapper.java +++ b/src/main/java/freemarker/ext/beans/BeansWrapper.java @@ -249,6 +249,13 @@ public class BeansWrapper implements RichObjectWrapper, WriteProtectable { * like in Java), which is hence never the one with the {@code Object} parameter type. For more details * about overloaded method selection changes see the version history in the FreeMarker Manual. * </li> + * <li> + * <p>2.3.24 (or higher): + * {@link Iterator}-s were always said to be non-empty when using {@code ?has_content} and such (i.e., + * operators that check emptiness without reading any elements). Now an {@link Iterator} counts as + * empty exactly if it has no elements left. (Note that this bug has never affected basic functionality, like + * {@code <#list ...>}.) + * </li> * </ul> * * <p>Note that the version will be normalized to the lowest version where the same incompatible @@ -793,6 +800,14 @@ public class BeansWrapper implements RichObjectWrapper, WriteProtectable { static boolean is2321Bugfixed(Version version) { return version.intValue() >= _TemplateAPI.VERSION_INT_2_3_21; } + + boolean is2324Bugfixed() { + return is2324Bugfixed(getIncompatibleImprovements()); + } + + static boolean is2324Bugfixed(Version version) { + return version.intValue() >= _TemplateAPI.VERSION_INT_2_3_24; + } /** * Returns the lowest version number that is equivalent with the parameter version. @@ -803,7 +818,9 @@ public class BeansWrapper implements RichObjectWrapper, WriteProtectable { if (incompatibleImprovements.intValue() < _TemplateAPI.VERSION_INT_2_3_0) { throw new IllegalArgumentException("Version must be at least 2.3.0."); } - return is2321Bugfixed(incompatibleImprovements) ? Configuration.VERSION_2_3_21 : Configuration.VERSION_2_3_0; + return is2324Bugfixed(incompatibleImprovements) ? Configuration.VERSION_2_3_24 + : is2321Bugfixed(incompatibleImprovements) ? Configuration.VERSION_2_3_21 + : Configuration.VERSION_2_3_0; } /** http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/main/java/freemarker/template/Configuration.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/template/Configuration.java b/src/main/java/freemarker/template/Configuration.java index fe178dc..c04f37a 100644 --- a/src/main/java/freemarker/template/Configuration.java +++ b/src/main/java/freemarker/template/Configuration.java @@ -756,6 +756,17 @@ public class Configuration extends Configurable implements Cloneable, ParserConf * (not <code>${...}</code>-s in general), like in <code><#assign s="Hello ${name}!"></code>, has * always used {@code incompatbileImprovement}-s 0 (2.3.0 in effect). Now it's fixed. * </li> + * <li><p> + * {@link DefaultObjectWrapper} has some minor changes with {@code incompatibleImprovements} 2.3.24; + * check them out at {@link DefaultObjectWrapper#DefaultObjectWrapper(Version)}. It's important to know + * that if you set the {@code object_wrapper} setting (to an other value than {@code "default"}), rather + * than leaving it on its default value, the {@code object_wrapper} won't inherit the + * {@code incompatibleImprovements} of the {@link Configuration}. In that case, if you want the 2.3.24 + * improvements of {@link DefaultObjectWrapper}, you have to set it in the {@link DefaultObjectWrapper} + * object itself too! (Note that it's OK to use a {@link DefaultObjectWrapper} with a different + * {@code incompatibleImprovements} version number than that of the {@link Configuration}, if that's + * really what you want.) + * </li> * </ul> * </li> * </ul> http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/main/java/freemarker/template/DefaultIteratorAdapter.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/template/DefaultIteratorAdapter.java b/src/main/java/freemarker/template/DefaultIteratorAdapter.java index 6ddbbcc..a99d5b7 100644 --- a/src/main/java/freemarker/template/DefaultIteratorAdapter.java +++ b/src/main/java/freemarker/template/DefaultIteratorAdapter.java @@ -45,7 +45,7 @@ public class DefaultIteratorAdapter extends WrappingTemplateModel implements Tem @SuppressFBWarnings(value="SE_BAD_FIELD", justification="We hope it's Seralizable") private final Iterator iterator; - private boolean iteratorOwned; + private boolean iteratorOwnedBySomeone; /** * Factory method for creating new adapter instances. @@ -83,7 +83,9 @@ public class DefaultIteratorAdapter extends WrappingTemplateModel implements Tem public TemplateModel next() throws TemplateModelException { if (!iteratorOwnedByMe) { - takeIteratorOwnership(); + checkNotOwner(); + iteratorOwnedBySomeone = true; + iteratorOwnedByMe = true; } if (!iterator.hasNext()) { @@ -97,19 +99,16 @@ public class DefaultIteratorAdapter extends WrappingTemplateModel implements Tem public boolean hasNext() throws TemplateModelException { // Calling hasNext may looks safe, but I have met sync. problems. if (!iteratorOwnedByMe) { - takeIteratorOwnership(); + checkNotOwner(); } return iterator.hasNext(); } - private void takeIteratorOwnership() throws TemplateModelException { - if (iteratorOwned) { + private void checkNotOwner() throws TemplateModelException { + if (iteratorOwnedBySomeone) { throw new TemplateModelException( "This collection value wraps a java.util.Iterator, thus it can be listed only once."); - } else { - iteratorOwned = true; - iteratorOwnedByMe = true; } } } http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/main/java/freemarker/template/DefaultObjectWrapper.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/template/DefaultObjectWrapper.java b/src/main/java/freemarker/template/DefaultObjectWrapper.java index 4bcf9bf..bda6f32 100644 --- a/src/main/java/freemarker/template/DefaultObjectWrapper.java +++ b/src/main/java/freemarker/template/DefaultObjectWrapper.java @@ -89,6 +89,11 @@ public class DefaultObjectWrapper extends freemarker.ext.beans.BeansWrapper { * <li>2.3.22 (or higher): The default value of * {@link #setUseAdaptersForContainers(boolean) useAdaptersForContainers} changes to * {@code true}.</li> + * <li>2.3.24 (or higher): When wrapping an {@link Iterator}, operations on it that only check if the + * collection is empty without reading an element from it, such as {@code ?has_content}, + * won't cause the a later iteration (or further emptiness check) to fail anymore. Earlier, in + * certain situations, the second operation has failed saying that the iterator "can be listed only + * once". * </ul> * * @since 2.3.21 http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/main/java/freemarker/template/SimpleCollection.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/template/SimpleCollection.java b/src/main/java/freemarker/template/SimpleCollection.java index baa5677..77f9755 100644 --- a/src/main/java/freemarker/template/SimpleCollection.java +++ b/src/main/java/freemarker/template/SimpleCollection.java @@ -112,7 +112,11 @@ implements TemplateCollectionModel, Serializable { public TemplateModel next() throws TemplateModelException { if (!iteratorOwnedByMe) { - takeIteratorOwnership(); + synchronized (SimpleCollection.this) { + checkIteratorOwned(); + iteratorOwned = true; + iteratorOwnedByMe = true; + } } if (!iterator.hasNext()) { @@ -126,23 +130,21 @@ implements TemplateCollectionModel, Serializable { public boolean hasNext() throws TemplateModelException { // Calling hasNext may looks safe, but I have met sync. problems. if (!iteratorOwnedByMe) { - takeIteratorOwnership(); + synchronized (SimpleCollection.this) { + checkIteratorOwned(); + } } return iterator.hasNext(); } - private void takeIteratorOwnership() throws TemplateModelException { - synchronized (SimpleCollection.this) { - if (iteratorOwned) { - throw new TemplateModelException( - "This collection value wraps a java.util.Iterator, thus it can be listed only once."); - } else { - iteratorOwned = true; - iteratorOwnedByMe = true; - } + private void checkIteratorOwned() throws TemplateModelException { + if (iteratorOwned) { + throw new TemplateModelException( + "This collection value wraps a java.util.Iterator, thus it can be listed only once."); } } + } } http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/manual/book.xml ---------------------------------------------------------------------- diff --git a/src/manual/book.xml b/src/manual/book.xml index 8030ffb..82f89ac 100644 --- a/src/manual/book.xml +++ b/src/manual/book.xml @@ -26160,6 +26160,40 @@ TemplateModel x = env.getVariable("x"); // get variable x</programlisting> </listitem> <listitem> + <para>Bug fixed: It wasn't well defined when a Java + <literal>Iterator</literal> counts as empty. Depending on what + <literal>ObjectWrapper</literal> you are using, one of these + fixes apply:</para> + + <itemizedlist> + <listitem> + <para><literal>DefaultObjectWrapper</literal> (fix is always + active): Operations on the <literal>Iterator</literal> that + only check if it's empty without reading an element from it, + such as <literal>?has_content</literal>, won't cause the a + later iteration (or further emptiness check) to fail + anymore. Earlier, in certain situations, the second + operation has failed saying that the iterator <quote>can be + listed only once</quote>.</para> + </listitem> + + <listitem> + <para><literal>BeansWrapper</literal> (when it's not + extended by <literal>DefaultObjectWrapper</literal>), if + it's <literal>incompatibleImprovements</literal> property is + set to 2.3.24 (or higher): <literal>Iterator</literal>-s + were always said to be non-empty when using + <literal>?has_content</literal> and such (i.e., operators + that check emptiness without reading any elements). Now an + <literal>Iterator</literal> counts as empty exactly if it + has no elements left. (Note that this bug has never affected + basic functionality, like <literal><#list + ...></literal>.)</para> + </listitem> + </itemizedlist> + </listitem> + + <listitem> <para>Bug fixed: The (rarely used) cause exception of <literal>ParseException</literal>-s wasn't set</para> </listitem> http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/test/java/freemarker/core/IteratorIssuesTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/freemarker/core/IteratorIssuesTest.java b/src/test/java/freemarker/core/IteratorIssuesTest.java new file mode 100644 index 0000000..78de020 --- /dev/null +++ b/src/test/java/freemarker/core/IteratorIssuesTest.java @@ -0,0 +1,149 @@ +package freemarker.core; + +import java.util.Arrays; +import java.util.Iterator; + +import org.junit.Test; + +import freemarker.ext.beans.BeansWrapper; +import freemarker.ext.beans.BeansWrapperBuilder; +import freemarker.template.Configuration; +import freemarker.template.DefaultObjectWrapper; +import freemarker.template.DefaultObjectWrapperBuilder; +import freemarker.test.TemplateTest; + +public class IteratorIssuesTest extends TemplateTest { + + private static final String FTL_HAS_CONTENT_AND_LIST + = "<#if it?hasContent><#list it as i>${i}</#list><#else>empty</#if>"; + private static final String OUT_HAS_CONTENT_AND_LIST_ABC = "abc"; + private static final String OUT_HAS_CONTENT_AND_LIST_EMPTY = "empty"; + + private static final String FTL_LIST_AND_HAS_CONTENT + = "<#list it as i>${i}${it?hasContent?then('+', '-')}</#list>"; + private static final String OUT_LIST_AND_HAS_CONTENT_BW_WRONG = "a+b+c+"; + private static final String OUT_LIST_AND_HAS_CONTENT_BW_GOOD = "a+b+c-"; + + @Test + public void testHasContentAndListDOW230() throws Exception { + addToDataModel("it", getDOW230().wrap(getAbcIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_ABC); + + addToDataModel("it", getDOW230().wrap(getEmptyIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_EMPTY); + } + + @Test + public void testHasContentAndListDOW2323() throws Exception { + addToDataModel("it", getDOW2323().wrap(getAbcIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_ABC); + + addToDataModel("it", getDOW2323().wrap(getEmptyIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_EMPTY); + } + + @Test + public void testHasContentAndListDOW2324() throws Exception { + addToDataModel("it", getDOW2324().wrap(getAbcIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_ABC); + + addToDataModel("it", getDOW2324().wrap(getEmptyIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_EMPTY); + } + + @Test + public void testHasContentAndListBW230() throws Exception { + addToDataModel("it", getBW230().wrap(getAbcIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_ABC); + + addToDataModel("it", getBW230().wrap(getEmptyIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, ""); + } + + @Test + public void testHasContentAndListBW2323() throws Exception { + addToDataModel("it", getBW2323().wrap(getAbcIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_ABC); + + addToDataModel("it", getBW230().wrap(getEmptyIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, ""); + } + + @Test + public void testHasContentAndListBW2324() throws Exception { + addToDataModel("it", getBW2324().wrap(getAbcIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_ABC); + + addToDataModel("it", getBW2324().wrap(getEmptyIt())); + assertOutput(FTL_HAS_CONTENT_AND_LIST, OUT_HAS_CONTENT_AND_LIST_EMPTY); + } + + @Test + public void testListAndHasContentDOW230() throws Exception { + addToDataModel("it", getDOW230().wrap(getAbcIt())); + assertErrorContains(FTL_LIST_AND_HAS_CONTENT, "can be listed only once"); + } + + @Test + public void testListAndHasContentDOW2323() throws Exception { + addToDataModel("it", getDOW2323().wrap(getAbcIt())); + assertErrorContains(FTL_LIST_AND_HAS_CONTENT, "can be listed only once"); + } + + @Test + public void testListAndHasContentDOW2324() throws Exception { + addToDataModel("it", getDOW2324().wrap(getAbcIt())); + assertErrorContains(FTL_LIST_AND_HAS_CONTENT, "can be listed only once"); + } + + @Test + public void testListAndHasContentBW230() throws Exception { + addToDataModel("it", getBW230().wrap(getAbcIt())); + assertOutput(FTL_LIST_AND_HAS_CONTENT, OUT_LIST_AND_HAS_CONTENT_BW_WRONG); + } + + @Test + public void testListAndHasContentBW2323() throws Exception { + addToDataModel("it", getBW2323().wrap(getAbcIt())); + assertOutput(FTL_LIST_AND_HAS_CONTENT, OUT_LIST_AND_HAS_CONTENT_BW_WRONG); + } + + @Test + public void testListAndHasContentBW2324() throws Exception { + addToDataModel("it", getBW2324().wrap(getAbcIt())); + assertOutput(FTL_LIST_AND_HAS_CONTENT, OUT_LIST_AND_HAS_CONTENT_BW_GOOD); + } + + private Iterator getAbcIt() { + return Arrays.asList(new String[] { "a", "b", "c" }).iterator(); + } + + private Iterator getEmptyIt() { + return Arrays.asList(new String[] { }).iterator(); + } + + private DefaultObjectWrapper getDOW230() { + return new DefaultObjectWrapperBuilder(Configuration.VERSION_2_3_0).build(); + } + + private DefaultObjectWrapper getDOW2323() { + return new DefaultObjectWrapperBuilder(Configuration.VERSION_2_3_23).build(); + } + + private DefaultObjectWrapper getDOW2324() { + return new DefaultObjectWrapperBuilder(Configuration.VERSION_2_3_24).build(); + } + + private BeansWrapper getBW230() { + return new BeansWrapperBuilder(Configuration.VERSION_2_3_0).build(); + } + + private BeansWrapper getBW2323() { + return new BeansWrapperBuilder(Configuration.VERSION_2_3_23).build(); + } + + private BeansWrapper getBW2324() { + return new BeansWrapperBuilder(Configuration.VERSION_2_3_24).build(); + } + +} http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/eef30af2/src/test/java/freemarker/template/DefaultObjectWrapperTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/freemarker/template/DefaultObjectWrapperTest.java b/src/test/java/freemarker/template/DefaultObjectWrapperTest.java index b398965..f94f2d9 100644 --- a/src/test/java/freemarker/template/DefaultObjectWrapperTest.java +++ b/src/test/java/freemarker/template/DefaultObjectWrapperTest.java @@ -82,7 +82,7 @@ public class DefaultObjectWrapperTest { expected.add(Configuration.VERSION_2_3_21); expected.add(Configuration.VERSION_2_3_22); expected.add(Configuration.VERSION_2_3_22); // no non-BC change in 2.3.23 - expected.add(Configuration.VERSION_2_3_22); // no non-BC change in 2.3.24 + expected.add(Configuration.VERSION_2_3_24); List<Version> actual = new ArrayList<Version>(); for (int i = _TemplateAPI.VERSION_INT_2_3_0; i <= Configuration.getVersion().intValue(); i++) {
