fixed Portlet request mapping priorities in cross-controller case

Issue: SPR-9303 Issue: SPR-9605
lehoon · Aug 29, 2012 · 95adf1d · 95adf1d
1 parent 049b944
commit 95adf1d
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 29 deletions.
diff --git a/...src/main/java/org/springframework/web/portlet/handler/AbstractMapBasedHandlerMapping.java b/...src/main/java/org/springframework/web/portlet/handler/AbstractMapBasedHandlerMapping.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2008 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -76,16 +76,19 @@ protected Object getHandlerInternal(PortletRequest request) throws Exception {
 		if (handler instanceof Map) {
 			Map<PortletRequestMappingPredicate, Object> predicateMap =
 					(Map<PortletRequestMappingPredicate, Object>) handler;
-			List<PortletRequestMappingPredicate> predicates =
-					new LinkedList<PortletRequestMappingPredicate>(predicateMap.keySet());
-			Collections.sort(predicates);
-			for (PortletRequestMappingPredicate predicate : predicates) {
+			List<PortletRequestMappingPredicate> filtered = new LinkedList<PortletRequestMappingPredicate>();
+			for (PortletRequestMappingPredicate predicate : predicateMap.keySet()) {
 				if (predicate.match(request)) {
-					predicate.validate(request);
-					return predicateMap.get(predicate);
+					filtered.add(predicate);
 				}
 			}
-			return null;
+			if (filtered.isEmpty()) {
+				return null;
+			}
+			Collections.sort(filtered);
+			PortletRequestMappingPredicate predicate = filtered.get(0);
+			predicate.validate(request);
+			return predicateMap.get(predicate);
 		}
 		return handler;
 	}

diff --git a/.../java/org/springframework/web/portlet/mvc/annotation/DefaultAnnotationHandlerMapping.java b/.../java/org/springframework/web/portlet/mvc/annotation/DefaultAnnotationHandlerMapping.java
@@ -272,7 +272,7 @@ private interface SpecialRequestTypePredicate {
 
 	private static abstract class AbstractParameterMappingPredicate implements PortletRequestMappingPredicate {
 
-		protected final String[] params;
+		private final String[] params;
 
 		public AbstractParameterMappingPredicate(String[] params) {
 			this.params = params;
@@ -281,6 +281,17 @@ public AbstractParameterMappingPredicate(String[] params) {
 		public boolean match(PortletRequest request) {
 			return PortletAnnotationMappingUtils.checkParameters(this.params, request);
 		}
+
+		protected int compareParams(AbstractParameterMappingPredicate other) {
+			return new Integer(other.params.length).compareTo(this.params.length);
+		}
+
+		protected int compareParams(Object other) {
+			if (other instanceof AbstractParameterMappingPredicate) {
+				return compareParams((AbstractParameterMappingPredicate) other);
+			}
+			return 0;
+		}
 	}
 
 
@@ -318,10 +329,7 @@ public void validate(PortletRequest request) throws PortletException {
 		}
 
 		public int compareTo(Object other) {
-			if (other instanceof AbstractParameterMappingPredicate) {
-				return new Integer(((AbstractParameterMappingPredicate) other).params.length).compareTo(this.params.length);
-			}
-			return (other instanceof SpecialRequestTypePredicate ? -1 : 0);
+			return (other instanceof SpecialRequestTypePredicate ? -1 : compareParams(other));
 		}
 	}
 
@@ -336,13 +344,7 @@ public void validate(PortletRequest request) throws PortletException {
 		}
 
 		public int compareTo(Object other) {
-			if (other instanceof SpecialRequestTypePredicate) {
-				return 1;
-			}
-			else if (other instanceof AbstractParameterMappingPredicate) {
-				return new Integer(((AbstractParameterMappingPredicate) other).params.length).compareTo(this.params.length);
-			}
-			return 0;
+			return (other instanceof SpecialRequestTypePredicate ? 1 : compareParams(other));
 		}
 	}
 
@@ -378,10 +380,10 @@ else if (other instanceof ActionMappingPredicate) {
 					return (hasActionName ? -1 : 1);
 				}
 				else {
-					return new Integer(otherAction.params.length).compareTo(this.params.length);
+					return compareParams(otherAction);
 				}
 			}
-			return (other instanceof SpecialRequestTypePredicate ? 0 : -1);
+			return (other instanceof SpecialRequestTypePredicate ? compareParams(other) : -1);
 		}
 	}
 
@@ -417,10 +419,10 @@ else if (other instanceof RenderMappingPredicate) {
 					return (hasWindowState ? -1 : 1);
 				}
 				else {
-					return new Integer(otherRender.params.length).compareTo(this.params.length);
+					return compareParams(otherRender);
 				}
 			}
-			return (other instanceof SpecialRequestTypePredicate ? 0 : -1);
+			return (other instanceof SpecialRequestTypePredicate ? compareParams(other) : -1);
 		}
 	}
 
@@ -443,8 +445,8 @@ public void validate(PortletRequest request) {
 
 		public int compareTo(Object other) {
 			if (other instanceof ResourceMappingPredicate) {
-				boolean hasResourceId = "".equals(this.resourceId);
-				boolean otherHasResourceId = "".equals(((ResourceMappingPredicate) other).resourceId);
+				boolean hasResourceId = !"".equals(this.resourceId);
+				boolean otherHasResourceId = !"".equals(((ResourceMappingPredicate) other).resourceId);
 				if (hasResourceId != otherHasResourceId) {
 					return (hasResourceId ? -1 : 1);
 				}
@@ -478,8 +480,8 @@ public void validate(PortletRequest request) {
 
 		public int compareTo(Object other) {
 			if (other instanceof EventMappingPredicate) {
-				boolean hasEventName = "".equals(this.eventName);
-				boolean otherHasEventName = "".equals(((EventMappingPredicate) other).eventName);
+				boolean hasEventName = !"".equals(this.eventName);
+				boolean otherHasEventName = !"".equals(((EventMappingPredicate) other).eventName);
 				if (hasEventName != otherHasEventName) {
 					return (hasEventName ? -1 : 1);
 				}

diff --git a/...va/org/springframework/web/portlet/mvc/annotation/Portlet20AnnotationControllerTests.java b/...va/org/springframework/web/portlet/mvc/annotation/Portlet20AnnotationControllerTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 the original author or authors.
+ * Copyright 2002-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -33,6 +33,7 @@
 import javax.portlet.PortletSession;
 import javax.portlet.RenderRequest;
 import javax.portlet.RenderResponse;
+import javax.portlet.ResourceRequest;
 import javax.portlet.ResourceResponse;
 import javax.portlet.StateAwareResponse;
 import javax.portlet.WindowState;
@@ -694,6 +695,32 @@ protected ApplicationContext createPortletApplicationContext(ApplicationContext
 		assertEquals("myDefaultResource", resourceResponse.getContentAsString());
 	}
 
+	@Test
+	public void testPredicatePriorityComparisonAcrossControllers() throws Exception {
+		DispatcherPortlet portlet = new DispatcherPortlet() {
+			protected ApplicationContext createPortletApplicationContext(ApplicationContext parent) throws BeansException {
+				StaticPortletApplicationContext wac = new StaticPortletApplicationContext();
+				// The order of handler registration is important to get
+				// the collection with [Render,Action,Render] predicates
+				wac.registerSingleton("firstController", FirstController.class);
+				wac.registerSingleton("secondController", SecondController.class);
+				wac.registerSingleton("handlerMapping", DefaultAnnotationHandlerMapping.class);
+				wac.registerSingleton("handlerAdapter", AnnotationMethodHandlerAdapter.class);
+				wac.setPortletContext(new MockPortletContext());
+				AnnotationConfigUtils.registerAnnotationConfigProcessors(wac);
+				wac.refresh();
+				return wac;
+			}
+		};
+		portlet.init(new MockPortletConfig());
+
+		// Prepare render request with 'page=baz' parameters
+		MockRenderRequest request = new MockRenderRequest(PortletMode.VIEW);
+		MockRenderResponse response = new MockRenderResponse();
+		request.addParameter("page", "baz");
+		portlet.render(request, response);
+	}
+
 
 	/*
 	 * Controllers
@@ -1169,4 +1196,34 @@ public void render(String viewName, Map model, PortletRequest request, MimeRespo
 		}
 	}
 
+
+	@RequestMapping(value="view")
+	public static class FirstController {
+
+		@RenderMapping
+		public String renderBar() {
+			throw new UnsupportedOperationException("Should not be called");
+		}
+
+		@ActionMapping("xyz")
+		public void processXyz() {
+			throw new UnsupportedOperationException("Should not be called");
+		}
+	}
+
+
+	@RequestMapping(value="view")
+	public static class SecondController {
+
+		@ResourceMapping
+		public void processResource(ResourceRequest request, ResourceResponse response) {
+			throw new UnsupportedOperationException("Should not be called");
+		}
+
+		@RenderMapping(params="page=baz")
+		public String renderBaz() {
+			return "SUCCESS";
+		}
+	}
+
 }