WICKET-6823: Add a container to AbstractTransformerBehavior to support multiple instances per component.

wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java

@@ -16,6 +16,11 @@
  */
 package org.apache.wicket.markup.transformer;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Objects;
+
 import org.apache.wicket.Component;
 import org.apache.wicket.Page;
 import org.apache.wicket.WicketRuntimeException;
@@ -26,17 +31,114 @@
 import org.apache.wicket.request.http.WebResponse;
 
 /**
- * A {@link Behavior} which can be added to any component. It allows to post-process (transform) the
+ * A {@link Behavior} which can be added to any component, allowing to post-process (transform) the
  * markup generated by the component.
- * 
+ * <p>
+ * There's one important limitation with the current implementation: Multiple different instances of
+ * this behavior CAN NOT be assigned to the same component! If one whiches to do so, the contained
+ * container needs to be used to wrap existing behaviors and that container needs to be added to the
+ * component instead. The current implementation of works with temporary responses, but does not
+ * support nesting itself properly, which results in missing rendered output and most likely broken
+ * HTML documents in the end.
+ * </p>
  * @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer
- * 
+ * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+ *
  * @author Juergen Donnerstag
  */
 public abstract class AbstractTransformerBehavior extends Behavior implements ITransformer
 {
 	private static final long serialVersionUID = 1L;
 
+	/**
+	 * Container to apply multiple {@link AbstractTransformerBehavior} to some component.
+	 * <p>
+	 * This container is by design NOT about multiple arbitrary transformations, but really only for the
+	 * one use case supporting multiple instances of {@link AbstractTransformerBehavior} on one and the
+	 * same component. The current implementation of that works with temporary responses, but doesn't
+	 * support nesting itself properly in case multiple behaviors assigned to the same component, which
+	 * results in missing rendered output and most likely entirely broken HTML documents in the end.
+	 * </p>
+	 * <p>
+	 * The easiest workaround for that problem is simply introducing this container which users need to
+	 * use in those cases: An instance needs to be created with all transformers of interest in the
+	 * order they should be applied and the container takes care of doing so. Because the container is
+	 * an {@link AbstractTransformerBehavior} itself, things simply work like with individual behaviors,
+	 * while response handling is only managed by the container. So when used with this container, the
+	 * callbacks of the maintained instances like {@link AbstractTransformerBehavior#afterRender(Component)}
+	 * etc., are NOT used anymore! OTOH, the individual behaviors stay useful without the container as
+	 * well.
+	 * </p>
+	 * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+	 */
+	public static class Multi extends AbstractTransformerBehavior
+	{
+		private static final long serialVersionUID = 1L;
+
+		/**
+		 * All transformers which need to be applied in the order they need to be applied.
+		 */
+		private final List<AbstractTransformerBehavior> transes;
+
+		/**
+		 * CTOR simply storing the given transformers.
+		 *
+		 * @param transes, which must not be {@code null} or empty, as neither make sense here.
+		 */
+		private Multi(List<AbstractTransformerBehavior> transes)
+		{
+			if ((transes == null) || transes.isEmpty())
+			{
+				throw new IllegalArgumentException("No transformers given.");
+			}
+
+			this.transes = transes;
+		}
+
+		@Override
+		public CharSequence transform(Component component, CharSequence output) throws Exception
+		{
+			CharSequence retVal = output;
+			for (AbstractTransformerBehavior trans : this.transes)
+			{
+				retVal = trans.transform(component, retVal);
+			}
+
+			return retVal;
+		}
+
+		/**
+		 * Create a new container with the given transformers and with keeping their order.
+		 * <p>
+		 * This factory expects multiple individual transformers already, because creating a container
+		 * with less doesn't make too much sense and users should reconsider then if this container is
+		 * of use at all. In most cases users do have individual transformers to apply only anyway and
+		 * don't need to provide a collection themself this way. OTOH, a collection could be empty or
+		 * contain only one element would then defeat the purpose of this container again.
+		 * </p>
+		 * @param first First transformer to apply.
+		 * @param second Second transformer to apply.
+		 * @param moreIf All other transformers to apply, if at all, in given order.
+		 * @return A container with multiple transformers being applied.
+		 */
+		public static Multi newFor(	AbstractTransformerBehavior		first,
+									AbstractTransformerBehavior		second,
+									AbstractTransformerBehavior...	moreIf)
+		{
+			List<AbstractTransformerBehavior> transes = new ArrayList<>();
+
+			transes.add(Objects.requireNonNull(first,	"No first transformer given."));
+			transes.add(Objects.requireNonNull(second,	"No second transformer given."));
+
+			if ((moreIf != null) && (moreIf.length > 0))
+			{
+				transes.addAll(Arrays.asList(moreIf));
+			}
+
+			return new Multi(transes);
+		}
+	}
+
 	/**
 	 * The request cycle's response before the transformation.
 	 */
@@ -45,10 +147,10 @@ public abstract class AbstractTransformerBehavior extends Behavior implements IT
 	/**
 	 * Create a new response object which is used to store the markup generated by the child
 	 * objects.
-	 * 
+	 *
 	 * @param originalResponse
 	 *            the original web response or {@code null} if it isn't a {@link WebResponse}
-	 * 
+	 *
 	 * @return Response object. Must not be null
 	 */
 	protected BufferedWebResponse newResponse(final WebResponse originalResponse)

wicket-core/src/test/java/org/apache/wicket/markup/transformer/AbstractTransformerBehaviorTest.java

@@ -16,6 +16,8 @@
  */
 package org.apache.wicket.markup.transformer;
 
+import java.util.function.Function;
+
 import org.apache.wicket.Component;
 import org.apache.wicket.MarkupContainer;
 import org.apache.wicket.ajax.AjaxRequestTarget;
@@ -37,7 +39,10 @@ public class AbstractTransformerBehaviorTest extends WicketTestCase
 	@Test
 	public void responseTransformation()
 	{
-		TestPage testPage = new TestPage();
+		TestPage	testPage	= new TestPage();
+		String		replacement	= "replacement";
+
+		testPage.add(new Label(TestPage.LABEL_ID, TestPage.LABEL_VAL));
 		testPage.add(new AbstractTransformerBehavior()
 		{
 			/** */
@@ -47,11 +52,12 @@ public void responseTransformation()
 			public CharSequence transform(Component component, CharSequence output)
 				throws Exception
 			{
-				return output.toString().replace("to be replaced", "replacement");
+				return output.toString().replace(TestPage.LABEL_VAL, replacement);
 			}
 		});
+
 		tester.startPage(testPage);
-		assertTrue(tester.getLastResponseAsString().contains("replacement"));
+		assertTrue(tester.getLastResponseAsString().contains(replacement));
 	}
 
 	/**
@@ -69,7 +75,77 @@ public void transformationInAjaxRequest()
 		tester.clickLink("updateLabel", true);
 		tester.assertContains("ajax request");
 		tester.assertContainsNot("normal request");
+	}
 
+	/**
+	 * Test how multiple different transformers applied to the same component behave.
+	 * <p>
+	 * The current implementation of {@link AbstractTransformerBehavior} doesn't support multiple
+	 * instances on the same component, a container needs to be used explicitly instead. So make
+	 * sure the implementation is as expected, as otherwise the container might not be necessary at
+	 * all anymore, and that the container really works around the problem.
+	 * </p>
+	 * @see <a href="https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6823">JIRA issue</a>
+	 */
+	@Test
+	public void multiTransesSameComp()
+	{
+		TestPage	testPage	= new TestPage();
+		Label		label		= new Label(TestPage.LABEL_ID, TestPage.LABEL_VAL);
+
+		Function<String, AbstractTransformerBehavior> transProd = (val) ->
+		{
+			return new AbstractTransformerBehavior()
+			{
+				/** */
+				private static final long serialVersionUID = 1L;
+
+				@Override
+				public CharSequence transform(Component component, CharSequence output)
+					throws Exception
+				{
+					String outStr = output.toString();
+					if (outStr.contains(TestPage.LABEL_VAL))
+					{
+						return outStr.replace(TestPage.LABEL_VAL, val);
+					}
+
+					// Make somewhat sure to recognize that BOTH transformers have been applied.
+					return outStr.replaceAll
+					(
+						">(.+)</span>",
+						String.format(">$1%s</span>", val)
+					);
+				}
+			};
+		};
+
+		label.add(transProd.apply("foo"));
+		label.add(transProd.apply("bar"));
+
+		// Make sure the expected limited implementation is still available, which makes a container
+		// necessary only at all. If that has changed, the container might be removed as well.
+		testPage.add(label);
+		tester.startPage(testPage);
+		tester.isVisible(TestPage.LABEL_ID);
+		tester.assertContains(">foo</span>");
+		tester.assertContainsNot("</html>");
+
+		testPage	= new TestPage();
+		label		= new Label(TestPage.LABEL_ID, TestPage.LABEL_VAL);
+
+		label.add(AbstractTransformerBehavior.Multi.newFor
+		(
+			transProd.apply("foo"),
+			transProd.apply("bar")
+		));
+
+		// Maike sure that the container provided as workaround really fixes the problem.
+		testPage.add(label);
+		tester.startPage(testPage);
+		tester.isVisible(TestPage.LABEL_ID);
+		tester.assertContains(">foobar</span>");
+		tester.assertContains("</html>");
 	}
 
 	private static class AjaxTestPage extends WebPage implements IMarkupResourceStreamProvider
@@ -126,12 +202,14 @@ private static class TestPage extends WebPage implements IMarkupResourceStreamPr
 	{
 		private static final long serialVersionUID = 1L;
 
+		private static final String LABEL_ID	= "label";
+		private static final String LABEL_VAL	= "{to be replaced}";
+
 		@Override
 		public IResourceStream getMarkupResourceStream(MarkupContainer container,
 			Class<?> containerClass)
 		{
-			return new StringResourceStream("<html><body>{to be replaced}</body></html>");
+			return new StringResourceStream("<html><body><span wicket:id='label'></span></body></html>");
 		}
-
 	}
 }