Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portlet annotation handler mapping does is not working properly because of a flaw in predicate comparison [SPR-9303] #13941

Closed
spring-issuemaster opened this issue Apr 4, 2012 · 4 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Apr 4, 2012

Pavel Horal opened SPR-9303 and commented

Background

Class org.springframework.web.portlet.mvc.annotation.DefaultAnnotationHandlerMapping is using PortletMappingPredicates. These predicates are used to compare priorities of different mapping (e.g. mapping with attributes has a higher precedence than the one without parameters) as they support java.lang.Comparable interface. When selecting appropriate handler AbstractMapBasedHandlerMapping#getHandlerInternal is sorting the predicates (Collections.sort).

Example Implementation

I can have handler class as follows:

@RequestMapping("view")
public class Foo {

    @RenderMapping()
    public String renderBar() {
        // ...
    }

    @ActionMapping("xyz")
    public void processXyz() {
        // ...
    }

    @RenderMapping(params="page=baz")
    public String renderBaz() {
        // ...
    }

}

If I make request with parameter page=baz, the #renderBaz() method should be invoked. This might or might not happen (explanation bellow).

Predicate Comparison

Registering the example handler above (in DefaultAnnotationHandlerMapping) will result in creating three handler mapping predicates:

  1. RenderMappingPredicate for renderBar without parameters
  2. ActionMappingPredicate for processXyz without parameters
  3. RenderMappingPredicate for renderBaz with parameters

Now what happens in AbstractMapBasedHandlerMapping#getHandlerInternal is that the list with these predicates gets sorted and the first one matching the request is picked. The problem is that when they have exactly this order they are not sorted at all.

Comparison In Detail

This is compareTo() method implementation from RenderMappingPredicate:

public int compareTo(Object other) {
    if (other instanceof TypeLevelMappingPredicate) {
        return 1;
    }
    else if (other instanceof RenderMappingPredicate) {
        RenderMappingPredicate otherRender = (RenderMappingPredicate) other;
        boolean hasWindowState = "".equals(this.windowState);
        boolean otherHasWindowState = "".equals(otherRender.windowState);
        if (hasWindowState != otherHasWindowState) {
            return (hasWindowState ? -1 : 1);
        }
        else {
            return new Integer(otherRender.params.length).compareTo(this.params.length);
        }
    }
    return (other instanceof SpecialRequestTypePredicate ? 0 : -1);
}

The code in ActionMappingPredicate is almost identical. If you check this code against the predicates from the example you will get following comparisons:

  • predicates[0].compareTo(predicates[1]) = 0 (1st RenderMappingPredicate vs ActionMappingPredicate)
  • predicates[1].compareTo(predicates[2]) = 0 (ActionMappingPredicate vs 2nd RenderMappingPredicate)

The problem this issue is about is:

  • predicates[0].compareTo(predicates[2]) = 1 (1st RenderMappingPredicate vs 2nd RenderMappingPredicate)

Where Is The Issue?

The issue in comparison implementation is simple - parameter length should be compared first, regardles of the predicate type (you can see that the current implementation compares parameter length only in case predicates are of the same type).


Affects: 3.1.1, 3.1.2

Attachments:

Issue Links:

  • #14239 Wrong compareTo() implementation in Portlet mapping predicates
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 4, 2012

Pavel Horal commented

This issue should have CRITICAL priority!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 4, 2012

Pavel Horal commented

Test case showing the incorrect behavior.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 4, 2012

Pavel Horal commented

While writing the test case I find out, that this issue exists only when using two handlers/controllers without (or the same) TypeLevelPredicate. Having such configuration is AFAIK not prohibited, but it is definitely not common practice. Based on this fact the priority of this issue is not CRITICAL, but probably MINOR as it is now.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 7, 2012

Pavel Horal commented

Just found similar issue #12341 - it is using mapping accross two handlers without TypeLevelPredicate. What I've described here will also affect that case as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.