Hi Martin,
Sorry, but I dont think that fix will work. The problem is when compound
-> wrapper -> compound -> mount, that fix only does compound -> compound
-> mount.
I have coded up a solution, but I have not done tests yet.
I have attached the patch. Sorry, I still plan to do proper tests for this.
Thanks,
Jesse
On 22/09/2014 10:40, Martin Grigorov wrote:
Fixed.
The fix was much simpler !
See https://git-wip-us.apache.org/repos/asf?p=wicket.git;a=commit;h=f45ce896
Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov
On Wed, Sep 10, 2014 at 3:11 PM, Martin Grigorov <[email protected]>
wrote:
https://issues.apache.org/jira/browse/WICKET-5698
Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov
On Wed, Sep 10, 2014 at 3:08 PM, Martin Grigorov <[email protected]>
wrote:
Hi Jesse,
Do you work on this ?
I can take care of it if you don't have time.
Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov
On Thu, Sep 4, 2014 at 3:00 PM, Martin Grigorov <[email protected]>
wrote:
Hi Jesse,
To me 1) looks the simplest and the most flexible one.
Martin Grigorov
Wicket Training and Consulting
https://twitter.com/mtgrigorov
On Thu, Sep 4, 2014 at 1:12 PM, Jesse Long <[email protected]> wrote:
Hi Wicket Devs,
WebApplication.unmount() and/or CompoundRequestMapper.unmount() is
broken, and I want to fix it.
WebApplication.unmount() calls getRootRequestMapperAsCompound(), and
calls unmount() on that.
getRootRequestMapperAsCompound() checks if the root request mapper is
a compound, if not, wraps it in a compound, sets the compound as root and
returns the compound.
CompoundRequestMapper.unmount() identifies which of the mappers added
directly to the compound handle the url, and removes them.
The problem:
If the original root mapper was a single wrapper, or layer of wrappers,
with the actual mounted mapper wrapped some levels down, then the whole
wrapper is removed, not just the specific MountedMapper that is wrapped.
This has the effect of removing every single mapper, leaving root mapper as
an empty compound.
I would like to attempt to provide a patch to fix this, but would like
guidance on the approach. I have come up with three approaches:
1. Introduce interface IWrappedRequestMapper. This will be an interface
which has one method: IRequestMapper getWrappedRequestMapper(). We can then
have all wrapper mappers implement this and work down the tree to find the
correct MountedMapper (wicket 6) to remove.
2. Have WebApplication hold a reference to a specific
CompoundRequestMapper, and have all mount()/unmount() operations add and
remove from this mapper. This compound would need to be added to the
default list during init. This makes it complicated to work out how to do
things like have CryptoMapper not apply to mounted pages.
3. Add method unmount() to IRequestMapper, so that wrappers can
delegate. This obviously can only be done in wicket 7, but we're making
mounting a problem of every single request mapper, when not even
Application cares about mounting.
Any thoughts?
Thanks,
Jesse
diff --git a/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/CryptoMapper.java b/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/CryptoMapper.java
index be876fb..3fb7a54 100755
--- a/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/CryptoMapper.java
+++ b/wicket-core/src/main/java/org/apache/wicket/core/request/mapper/CryptoMapper.java
@@ -24,6 +24,7 @@ import org.apache.wicket.request.IRequestHandler;
import org.apache.wicket.request.IRequestMapper;
import org.apache.wicket.request.Request;
import org.apache.wicket.request.Url;
+import org.apache.wicket.request.mapper.IWrappedRequestMapper;
import org.apache.wicket.util.IProvider;
import org.apache.wicket.util.crypt.ICrypt;
import org.apache.wicket.util.crypt.ICryptFactory;
@@ -48,7 +49,7 @@ import org.slf4j.LoggerFactory;
* @author Jesse Long
* @author svenmeier
*/
-public class CryptoMapper implements IRequestMapper
+public class CryptoMapper implements IWrappedRequestMapper
{
private static final Logger log = LoggerFactory.getLogger(CryptoMapper.class);
@@ -145,7 +146,8 @@ public class CryptoMapper implements IRequestMapper
/**
* @return the wrapped root request mapper
*/
- protected final IRequestMapper getWrappedMapper()
+ @Override
+ public final IRequestMapper getWrappedMapper()
{
return wrappedMapper;
}
diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java b/wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java
index 52d41ca..65fdf77 100644
--- a/wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java
+++ b/wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java
@@ -17,6 +17,9 @@
package org.apache.wicket.protocol.http;
import java.io.UnsupportedEncodingException;
+import java.nio.charset.Charset;
+import java.util.Iterator;
+import java.util.Locale;
import javax.servlet.ServletContext;
import javax.servlet.http.HttpServletRequest;
@@ -62,6 +65,8 @@ import org.apache.wicket.request.handler.render.PageRenderer;
import org.apache.wicket.request.handler.render.WebPageRenderer;
import org.apache.wicket.request.http.WebRequest;
import org.apache.wicket.request.http.WebResponse;
+import org.apache.wicket.request.mapper.ICompoundRequestMapper;
+import org.apache.wicket.request.mapper.IWrappedRequestMapper;
import org.apache.wicket.request.resource.CssResourceReference;
import org.apache.wicket.request.resource.JavaScriptResourceReference;
import org.apache.wicket.request.resource.ResourceReference;
@@ -391,8 +396,83 @@ public abstract class WebApplication extends Application
{
path = path.substring(1);
}
- getRootRequestMapperAsCompound().unmount(path);
+
+ IRequestMapper mapper = getRootRequestMapper();
+
+ while (mapper instanceof IWrappedRequestMapper)
+ {
+ mapper = ((IWrappedRequestMapper)mapper).getWrappedMapper();
+ }
+
+ /*
+ * Only attempt to unmount is root request mapper is either a compound, or wraps a compound to avoid
+ * leaving the application with no mappers installed.
+ */
+ if (mapper instanceof ICompoundRequestMapper)
+ {
+ final Url url = Url.parse(path);
+
+ Request request = new Request()
+ {
+ @Override
+ public Url getUrl()
+ {
+ return url;
+ }
+
+ @Override
+ public Url getClientUrl()
+ {
+ return url;
+ }
+
+ @Override
+ public Locale getLocale()
+ {
+ return null;
+ }
+
+ @Override
+ public Charset getCharset()
+ {
+ return null;
+ }
+
+ @Override
+ public Object getContainerRequest()
+ {
+ return null;
+ }
+ };
+
+ unmountInternal((ICompoundRequestMapper)mapper, request);
+ }
}
+
+ private void unmountInternal(ICompoundRequestMapper parent, Request request)
+ {
+ for (IRequestMapper mapper : parent)
+ {
+ if (mapper.mapRequest(request) != null)
+ {
+ IRequestMapper actualMapper = mapper;
+
+ while (actualMapper instanceof IWrappedRequestMapper)
+ {
+ actualMapper = ((IWrappedRequestMapper)actualMapper).getWrappedMapper();
+ }
+
+ if (actualMapper instanceof ICompoundRequestMapper)
+ {
+ unmountInternal((ICompoundRequestMapper)actualMapper, request);
+ }
+ else
+ {
+ parent.remove(mapper);
+ }
+ }
+ }
+ }
/**
* Registers a replacement resource for the given javascript resource. This replacement can be
diff --git a/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java b/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java
index aaa1a3e..5e8de82 100644
--- a/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java
+++ b/wicket-core/src/main/java/org/apache/wicket/protocol/https/HttpsMapper.java
@@ -29,6 +29,7 @@ import org.apache.wicket.request.component.IRequestablePage;
import org.apache.wicket.request.cycle.RequestCycle;
import org.apache.wicket.request.http.WebRequest;
import org.apache.wicket.request.http.WebResponse;
+import org.apache.wicket.request.mapper.IWrappedRequestMapper;
import org.apache.wicket.util.collections.ClassMetaCache;
import org.apache.wicket.util.lang.Args;
@@ -61,7 +62,7 @@ import org.apache.wicket.util.lang.Args;
*
* @author igor
*/
-public class HttpsMapper implements IRequestMapper
+public class HttpsMapper implements IWrappedRequestMapper
{
private final HttpsConfig config;
private final IRequestMapper delegate;
@@ -79,6 +80,15 @@ public class HttpsMapper implements IRequestMapper
this.config = config;
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public IRequestMapper getWrappedMapper()
+ {
+ return delegate;
+ }
+
@Override
public final int getCompatibilityScore(Request request)
{
diff --git a/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java b/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java
index 46eff32..9f2658a 100644
--- a/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java
+++ b/wicket-core/src/main/java/org/apache/wicket/util/tester/BaseWicketTester.java
@@ -119,6 +119,7 @@ import org.apache.wicket.request.handler.render.PageRenderer;
import org.apache.wicket.request.handler.resource.ResourceReferenceRequestHandler;
import org.apache.wicket.request.http.WebRequest;
import org.apache.wicket.request.http.WebResponse;
+import org.apache.wicket.request.mapper.IWrappedRequestMapper;
import org.apache.wicket.request.mapper.parameter.PageParameters;
import org.apache.wicket.request.resource.IResource;
import org.apache.wicket.request.resource.ResourceReference;
@@ -2750,7 +2751,7 @@ public class BaseWicketTester
/**
*
*/
- private class TestRequestMapper implements IRequestMapper
+ private class TestRequestMapper implements IWrappedRequestMapper
{
private final IRequestMapper delegate;
@@ -2759,6 +2760,12 @@ public class BaseWicketTester
this.delegate = delegate;
}
+ @Override
+ public IRequestMapper getWrappedMapper()
+ {
+ return delegate;
+ }
+
@Override
public int getCompatibilityScore(Request request)
{
diff --git a/wicket-request/src/main/java/org/apache/wicket/request/mapper/CompoundRequestMapper.java b/wicket-request/src/main/java/org/apache/wicket/request/mapper/CompoundRequestMapper.java
index bbd6277..edfd77d 100644
--- a/wicket-request/src/main/java/org/apache/wicket/request/mapper/CompoundRequestMapper.java
+++ b/wicket-request/src/main/java/org/apache/wicket/request/mapper/CompoundRequestMapper.java
@@ -16,12 +16,10 @@
*/
package org.apache.wicket.request.mapper;
-import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
-import java.util.Locale;
import java.util.concurrent.CopyOnWriteArrayList;
import org.apache.wicket.request.IRequestHandler;
@@ -243,59 +241,8 @@ public class CompoundRequestMapper implements ICompoundRequestMapper
return mappers.iterator();
}
- @Override
- public void unmount(String path)
- {
- final Url url = Url.parse(path);
- final Request request = createRequest(url);
-
- for (IRequestMapper mapper : this)
- {
- if (mapper.mapRequest(request) != null)
- {
- remove(mapper);
- }
- }
- }
-
int size()
{
return mappers.size();
}
-
- Request createRequest(final Url url)
- {
- return new Request()
- {
- @Override
- public Url getUrl()
- {
- return url;
- }
-
- @Override
- public Locale getLocale()
- {
- return null;
- }
-
- @Override
- public Object getContainerRequest()
- {
- return null;
- }
-
- @Override
- public Url getClientUrl()
- {
- return url;
- }
-
- @Override
- public Charset getCharset()
- {
- return null;
- }
- };
- }
}
diff --git a/wicket-request/src/main/java/org/apache/wicket/request/mapper/ICompoundRequestMapper.java b/wicket-request/src/main/java/org/apache/wicket/request/mapper/ICompoundRequestMapper.java
index 0bb235c..2325290 100755
--- a/wicket-request/src/main/java/org/apache/wicket/request/mapper/ICompoundRequestMapper.java
+++ b/wicket-request/src/main/java/org/apache/wicket/request/mapper/ICompoundRequestMapper.java
@@ -41,12 +41,4 @@ public interface ICompoundRequestMapper extends IRequestMapper, Iterable<IReques
* @return {@code this} for chaining
*/
ICompoundRequestMapper remove(IRequestMapper mapper);
-
- /**
- * Unregisters all {@link IRequestMapper}s which would match on a this path
- *
- * @param path
- * the path to unmount
- */
- void unmount(String path);
}
\ No newline at end of file
diff --git a/wicket-request/src/main/java/org/apache/wicket/request/mapper/IWrappedRequestMapper.java b/wicket-request/src/main/java/org/apache/wicket/request/mapper/IWrappedRequestMapper.java
new file mode 100644
index 0000000..a9a8fce
--- /dev/null
+++ b/wicket-request/src/main/java/org/apache/wicket/request/mapper/IWrappedRequestMapper.java
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.request.mapper;
+
+import org.apache.wicket.request.IRequestMapper;
+
+/**
+ * A interface to be implemented by {@link IRequestMapper}s that wrap other {@link IRequestMapper}s.
+ * This allows the application to traverse the tree of request mappers to find a mounted mapper to remove
+ * when unmounting mounted mappers.
+ *
+ * @author Jesse Long
+ */
+public interface IWrappedRequestMapper
+ extends IRequestMapper
+{
+ /**
+ * Returns the wrapped {@link IRequestMapper}.
+ *
+ * @return The wrapped {@link IRequestMapper}.
+ */
+ IRequestMapper getWrappedMapper();
+}
diff --git a/wicket-request/src/main/java/org/apache/wicket/request/mapper/ParentPathReferenceRewriter.java b/wicket-request/src/main/java/org/apache/wicket/request/mapper/ParentPathReferenceRewriter.java
index c786685..2bd43c0 100755
--- a/wicket-request/src/main/java/org/apache/wicket/request/mapper/ParentPathReferenceRewriter.java
+++ b/wicket-request/src/main/java/org/apache/wicket/request/mapper/ParentPathReferenceRewriter.java
@@ -29,7 +29,7 @@ import org.apache.wicket.util.lang.Args;
*
* @author igor.vaynberg
*/
-public class ParentPathReferenceRewriter implements IRequestMapper
+public class ParentPathReferenceRewriter implements IWrappedRequestMapper
{
private final IProvider<String> escapeSequence;
private final IRequestMapper chain;
@@ -108,4 +108,10 @@ public class ParentPathReferenceRewriter implements IRequestMapper
{
return chain.getCompatibilityScore(request);
}
+
+ @Override
+ public IRequestMapper getWrappedMapper()
+ {
+ return chain;
+ }
}
diff --git a/wicket-request/src/test/java/org/apache/wicket/request/mapper/CompoundRequestMapperTest.java b/wicket-request/src/test/java/org/apache/wicket/request/mapper/CompoundRequestMapperTest.java
index a169dfc..977edf7 100644
--- a/wicket-request/src/test/java/org/apache/wicket/request/mapper/CompoundRequestMapperTest.java
+++ b/wicket-request/src/test/java/org/apache/wicket/request/mapper/CompoundRequestMapperTest.java
@@ -16,11 +16,6 @@
*/
package org.apache.wicket.request.mapper;
-import org.apache.wicket.request.IRequestHandler;
-import org.apache.wicket.request.IRequestMapper;
-import org.apache.wicket.request.Request;
-import org.apache.wicket.request.Url;
-import org.apache.wicket.request.handler.EmptyRequestHandler;
import org.apache.wicket.request.mapper.CompoundRequestMapper.MapperWithScore;
import org.junit.Assert;
import org.junit.Test;
@@ -30,72 +25,6 @@ import org.junit.Test;
*/
public class CompoundRequestMapperTest extends Assert
{
-
- private static final String MOUNT_PATH_3 = "mount/path/3";
- private static final String MOUNT_PATH_2 = "mount/path/2";
- private static final String MOUNT_PATH_1 = "mount/path/1";
-
- /**
- *
- */
- @Test
- public void unmount()
- {
- CompoundRequestMapper compound = new CompoundRequestMapper();
-
- compound.add(new MountMapper(MOUNT_PATH_1, new EmptyRequestHandler()));
- compound.add(new MountMapper(MOUNT_PATH_2, new EmptyRequestHandler()));
- compound.add(new MountMapper(MOUNT_PATH_3, new EmptyRequestHandler()));
-
- assertEquals(3, compound.size());
-
- compound.unmount(MOUNT_PATH_2);
- assertEquals(2, compound.size());
-
- assertTrue(
- "Mount path 1 should match",
- compound.mapRequest(compound.createRequest(Url.parse(MOUNT_PATH_1))) instanceof EmptyRequestHandler);
- assertNull("Mount path 2 should not match",
- compound.mapRequest(compound.createRequest(Url.parse(MOUNT_PATH_2))));
- assertTrue(
- "Mount path 3 should match",
- compound.mapRequest(compound.createRequest(Url.parse(MOUNT_PATH_3))) instanceof EmptyRequestHandler);
- }
-
- private static class MountMapper implements IRequestMapper
- {
- private final String path;
- private final IRequestHandler handler;
-
- public MountMapper(String path, EmptyRequestHandler handler)
- {
- this.path = path;
- this.handler = handler;
- }
-
- @Override
- public IRequestHandler mapRequest(Request request)
- {
- if (request.getUrl().toString().equals(path))
- {
- return handler;
- }
- return null;
- }
-
- @Override
- public int getCompatibilityScore(Request request)
- {
- return 0;
- }
-
- @Override
- public Url mapHandler(IRequestHandler requestHandler)
- {
- return null;
- }
- }
-
/**
* Test {@link MapperWithScore#compareTo(MapperWithScore)}.
*/
