From 95adf1dbeeb3172cc6fe21d542b27a288a3f0c0c Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 29 Aug 2012 22:02:45 +0200 Subject: [PATCH] fixed Portlet request mapping priorities in cross-controller case Issue: SPR-9303 Issue: SPR-9605 --- .../AbstractMapBasedHandlerMapping.java | 19 +++--- .../DefaultAnnotationHandlerMapping.java | 42 ++++++------- .../Portlet20AnnotationControllerTests.java | 59 ++++++++++++++++++- 3 files changed, 91 insertions(+), 29 deletions(-) diff --git a/spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/handler/AbstractMapBasedHandlerMapping.java b/spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/handler/AbstractMapBasedHandlerMapping.java index 14174097f9..8a803a5038 100644 --- a/spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/handler/AbstractMapBasedHandlerMapping.java +++ b/spring-webmvc-portlet/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 @@ public abstract class AbstractMapBasedHandlerMapping extends AbstractHandlerM if (handler instanceof Map) { Map predicateMap = (Map) handler; - List predicates = - new LinkedList(predicateMap.keySet()); - Collections.sort(predicates); - for (PortletRequestMappingPredicate predicate : predicates) { + List filtered = new LinkedList(); + 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/spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/DefaultAnnotationHandlerMapping.java b/spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/DefaultAnnotationHandlerMapping.java index 2f25cb1967..1fa3c6d025 100644 --- a/spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/DefaultAnnotationHandlerMapping.java +++ b/spring-webmvc-portlet/src/main/java/org/springframework/web/portlet/mvc/annotation/DefaultAnnotationHandlerMapping.java @@ -272,7 +272,7 @@ public class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp 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 class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp 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 class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp } 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 class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp } 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 @@ public class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp 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 @@ public class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp 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 class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp 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 class DefaultAnnotationHandlerMapping extends AbstractMapBasedHandlerMapp 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/spring-webmvc-portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/Portlet20AnnotationControllerTests.java b/spring-webmvc-portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/Portlet20AnnotationControllerTests.java index bc427c4d4b..cf59c16059 100644 --- a/spring-webmvc-portlet/src/test/java/org/springframework/web/portlet/mvc/annotation/Portlet20AnnotationControllerTests.java +++ b/spring-webmvc-portlet/src/test/java/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.PortletRequest; 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 @@ public class Portlet20AnnotationControllerTests { 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 class Portlet20AnnotationControllerTests { } } + + @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"; + } + } + }