Spring Framework
  1. Spring Framework
  2. SPR-7346

@RequestHeader negation expressions (e.g. !Accept=text/plain) are not applied

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.3
    • Fix Version/s: 3.1 M2
    • Component/s: Web
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      To reproduce:

      • Checkout https://src.springsource.org/svn/spring-samples/mvc-showcase
      • Deploy the application, view the root home page, and click "Mapping by not presence of header" link. You'll see "Mapped by regexp!", indicating the wrong @Controller method was matched. The method that should have been matched was MappingController.byHeaderNegation. The method looks like:
        @RequestMapping(value="/mapping/header", method=RequestMethod.GET, headers="!Accept=text/plain")
        public @ResponseBody String byHeaderNegation() {
        	return "Mapped by path + method + header with negation!";
        }
        

        You can confirm using Firebug that the request information contains what should be required to match ("text/plain" is not an Accept value and the resource URL and HTTP method is correct).

      1. ServletAnnotationMappingUtilsTest.java
        8 kB
        Eric Dalquist
      2. ServletAnnotationMappingUtilsTest.java
        7 kB
        Eric Dalquist
      3. SPR7346.patch
        0.9 kB
        Eric Dalquist

        Activity

        Hide
        Arjen Poutsma added a comment - - edited

        The negation operator only tests for header presence, not header value. I.e. !Accept=text/plain is interpreted as !Accept, and as such only applies to requests which don't have an Accept header. This is consistent with the way @RequestMapping.params works, and also documented, see http://static.springsource.org/spring/docs/3.0.x/javadoc-api/org/springframework/web/bind/annotation/RequestMapping.html#headers().

        We could add the functionality that you request, but I would suggest using a different syntax (Accept!=text/plain) to make it easier to parse. I'm changing this issue from a bug to an improvement to do so.

        Show
        Arjen Poutsma added a comment - - edited The negation operator only tests for header presence, not header value. I.e. !Accept=text/plain is interpreted as !Accept , and as such only applies to requests which don't have an Accept header. This is consistent with the way @RequestMapping.params works, and also documented, see http://static.springsource.org/spring/docs/3.0.x/javadoc-api/org/springframework/web/bind/annotation/RequestMapping.html#headers() . We could add the functionality that you request, but I would suggest using a different syntax ( Accept!=text/plain ) to make it easier to parse. I'm changing this issue from a bug to an improvement to do so.
        Hide
        Arjen Poutsma added a comment -

        Implemented, using the Unable to render embedded object: File (= operator shown above ("content-type) not found.=application/pdf"). Also implemented for RequestMapping.params().

        Show
        Arjen Poutsma added a comment - Implemented, using the Unable to render embedded object: File (= operator shown above ("content-type) not found. =application/pdf"). Also implemented for RequestMapping.params().
        Hide
        Keith Donald added a comment -

        The jsvadocs of request mapping indicate you should be able to test by value. Did you notice this?

        Show
        Keith Donald added a comment - The jsvadocs of request mapping indicate you should be able to test by value. Did you notice this?
        Hide
        Arjen Poutsma added a comment -

        You are able to test by value, just not in combination with the ! operator:

        Finally, "!My-Header" style expressions indicate that the specified header is not supposed to be present in the request.

        Anyway, it's implemented now (using the != operator), so I wonder why we are even having this discussion.

        Show
        Arjen Poutsma added a comment - You are able to test by value, just not in combination with the ! operator: Finally, "!My-Header" style expressions indicate that the specified header is not supposed to be present in the request. Anyway, it's implemented now (using the != operator), so I wonder why we are even having this discussion.
        Hide
        Christopher Taylor added a comment -

        negated header expressions are not evaluated correctly. I have these two controller methods:

        
        @RequestMapping(method = RequestMethod.POST, headers="content-type!=application/xml")
        public String create(...) {
        ...
        }
        
        @RequestMapping(method = RequestMethod.POST, headers="content-type=application/xml")
        public ResponseEntity<String> create(...) {
        ...
        }
        

        If the content-type header on the request is set to application/xml, the first controller is invoked.

        Using a debugger, I've traced this to faulty logic in ServletAnnotationMappingUtils.checkHeaders():
        For the first method, the variable 'found' is true and the variable 'negated' is true for a request with the header "Content-Type: application/xml". However, the check

        if(!found) {
            return negated;
        }
        

        causes the enclosing loop to continue, so that the method eventually ends up at the return true; on the last line, causing the wrong result to be returned.

        ServletAnnotationMappingUtils.checkHeaders (correctly) also returns true for the second controller method. Control flow then returns to AnnotationMethodHandlerAdapter$ServletHandlerMethodResolver.resolveHnadlerMethod. RequestSpecificMappingInfoComparator and Collections.sort don't change the list of matches, so the first method is incorrectly returned as the controller method.

        If I change my controller class and reverse the method definitions, the other create method is chosen. I suspect this bug can be reproduced by reversing the method definition order in ServletAnnotationControllerTests$NegatedContentTypeHeadersController, which should cause ServletAnnotationControllerTests.negatedContentTypeHeaders to fail.

        Show
        Christopher Taylor added a comment - negated header expressions are not evaluated correctly. I have these two controller methods: @RequestMapping(method = RequestMethod.POST, headers= "content-type!=application/xml" ) public String create(...) { ... } @RequestMapping(method = RequestMethod.POST, headers= "content-type=application/xml" ) public ResponseEntity< String > create(...) { ... } If the content-type header on the request is set to application/xml, the first controller is invoked. Using a debugger, I've traced this to faulty logic in ServletAnnotationMappingUtils.checkHeaders(): For the first method, the variable 'found' is true and the variable 'negated' is true for a request with the header "Content-Type: application/xml". However, the check if (!found) { return negated; } causes the enclosing loop to continue, so that the method eventually ends up at the return true; on the last line, causing the wrong result to be returned. ServletAnnotationMappingUtils.checkHeaders (correctly) also returns true for the second controller method. Control flow then returns to AnnotationMethodHandlerAdapter$ServletHandlerMethodResolver.resolveHnadlerMethod . RequestSpecificMappingInfoComparator and Collections.sort don't change the list of matches, so the first method is incorrectly returned as the controller method. If I change my controller class and reverse the method definitions, the other create method is chosen. I suspect this bug can be reproduced by reversing the method definition order in ServletAnnotationControllerTests$NegatedContentTypeHeadersController , which should cause ServletAnnotationControllerTests.negatedContentTypeHeaders to fail.
        Hide
        Eric Dalquist added a comment -

        Here is a unit test that verifies the logic problem in checkHeaders. The test also contains an updated version of checkHeaders that fixes the problem. I'll look at checking out the latest source as well and providing a true .patch file

        Show
        Eric Dalquist added a comment - Here is a unit test that verifies the logic problem in checkHeaders. The test also contains an updated version of checkHeaders that fixes the problem. I'll look at checking out the latest source as well and providing a true .patch file
        Hide
        Eric Dalquist added a comment -

        Here is a patch to resolve the issue and an update to the test that also tests and fixes the negation operator for the media type headers

        Show
        Eric Dalquist added a comment - Here is a patch to resolve the issue and an update to the test that also tests and fixes the negation operator for the media type headers
        Hide
        Arjen Poutsma added a comment -

        Fixed as part of SPR-7353

        Show
        Arjen Poutsma added a comment - Fixed as part of SPR-7353

          People

          • Assignee:
            Arjen Poutsma
            Reporter:
            Keith Donald
            Last updater:
            Trevor Marshall
          • Votes:
            4 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              2 years, 46 weeks, 1 day ago

              Time Tracking

              Estimated:
              Original Estimate - Not Specified
              Not Specified
              Remaining:
              Remaining Estimate - 0d
              0d
              Logged:
              Time Spent - 0.5h
              0.5h