diff --git a/wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java b/wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java index 0359bbbcd61..bb3e6d062a7 100644 --- a/wicket-core/src/main/java/org/apache/wicket/markup/transformer/AbstractTransformerBehavior.java +++ b/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. - * + *

+ * 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 wishes 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 works with temporary responses, but doesn't support + * nesting itself properly, which results in missing rendered output and most likely broken HTML + * documents in the end. + *

* @see org.apache.wicket.markup.transformer.AbstractOutputTransformerContainer - * + * * @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. + *

+ * 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 makes use of temporary responses, + * but doesn't support nesting itself properly if multiple behaviors get assigned to the same + * component. That results in missing rendered output and most likely entirely broken HTML. + *

+ *

+ * The easiest workaround for that problem is using this container 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. Being an {@link AbstractTransformerBehavior} itself, + * things simply work like with individual behaviors, while response handling is managed by the + * container only. This means that callbacks of the internally maintained instances, like + * {@link AbstractTransformerBehavior#afterRender(Component)} etc., are NOT used anymore! OTOH, + * this way the behaviors stay individually useful without the container as well. + *

+ * @see JIRA issue + */ + public static class Multi extends AbstractTransformerBehavior + { + private static final long serialVersionUID = 1L; + + /** + * All transformers which need to be applied in the order given by the user, which is the + * same order as processed by the container in the end. + */ + private final List transformers; + + /** + * Constructor simply storing the given transformers. + * + * @param transformers, which must not be {@code null} or empty. Wouldn't makes sense here. + */ + private Multi(List transformers) + { + if ((transformers == null) || transformers.isEmpty()) + { + throw new IllegalArgumentException("No transformers given."); + } + + this.transformers = transformers; + } + + @Override + public CharSequence transform(Component component, CharSequence output) throws Exception + { + CharSequence retVal = output; + for (AbstractTransformerBehavior trans : this.transformers) + { + retVal = trans.transform(component, retVal); + } + + return retVal; + } + + /** + * Create a new container with the given transformers and with keeping their order. + *

+ * This factory expects multiple individual transformers already, as creating a container + * with less of those 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, contain only one element etc. and would then defeat the purpose of this + * container again. + *

+ * @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 of( AbstractTransformerBehavior first, + AbstractTransformerBehavior second, + AbstractTransformerBehavior... moreIf) + { + List transformers = new ArrayList<>(); + + transformers.add(Objects.requireNonNull(first, "No first transformer given.")); + transformers.add(Objects.requireNonNull(second, "No second transformer given.")); + + if ((moreIf != null) && (moreIf.length > 0)) + { + transformers.addAll(Arrays.asList(moreIf)); + } + + return new Multi(transformers); + } + } + /** * 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) diff --git a/wicket-core/src/test/java/org/apache/wicket/markup/transformer/AbstractTransformerBehaviorTest.java b/wicket-core/src/test/java/org/apache/wicket/markup/transformer/AbstractTransformerBehaviorTest.java index adc30243a89..e70a25df523 100644 --- a/wicket-core/src/test/java/org/apache/wicket/markup/transformer/AbstractTransformerBehaviorTest.java +++ b/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. + *

+ * 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. + *

+ * @see JIRA issue + */ + @Test + public void multiTransesSameComp() + { + TestPage testPage = new TestPage(); + Label label = new Label(TestPage.LABEL_ID, TestPage.LABEL_VAL); + + Function transformerProducer = (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 + ( + ">(.+)", + String.format(">$1%s", val) + ); + } + }; + }; + + label.add(transformerProducer.apply("foo")); + label.add(transformerProducer.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"); + tester.assertContainsNot(""); + + testPage = new TestPage(); + label = new Label(TestPage.LABEL_ID, TestPage.LABEL_VAL); + + label.add(AbstractTransformerBehavior.Multi.of + ( + transformerProducer.apply("foo"), + transformerProducer.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"); + tester.assertContains(""); } 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("{to be replaced}"); + return new StringResourceStream(""); } - } }