Sling
  1. Sling
  2. SLING-1974

Accept header issues in the Sling POST Servlet

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: Servlets Post 2.1.0
    • Fix Version/s: None
    • Component/s: Servlets
    • Labels:
      None

      Description

      As of SLING-1336 the Sling POST Servlet can interpret the Accept request header to select what response content type to render.

      Unfortunately that handling seems broken as for an Accept header like (as generated by FireFox with the JSONovich plugin installed) :

      Accept: text/html,application/xhtml+xml,application/xml;q=0.9,/;q=0.8,application/json

      the JSON response might be selected but sometimes also text/html (we can observe both behaviours for different server platforms with our application).

      The Accept header should probably consider equivalent q values (as for text/html and application/json in the example) to solve the tie by selecting the first type list; thus text/html in this example.

      1. SLING-1974.patch
        3 kB
        Felix Meschberger

        Activity

        Hide
        Jukka Zitting added a comment -

        The HTTP spec doesn't specify any ordering for the values and some clients may use a non-deterministic ordering (e.g. hash set), so relying on the ordering might cause unexpected results in some cases. A better approach might be to use explicit server-side preferences (like text/html over application/json) or alphabetic ordering as a deterministic fallback.

        See also http://www.neeraj.name/2010/11/23/mime-type-resolution-in-rails.html (and especially Erik's comment) for a good discussion of related issues.

        Show
        Jukka Zitting added a comment - The HTTP spec doesn't specify any ordering for the values and some clients may use a non-deterministic ordering (e.g. hash set), so relying on the ordering might cause unexpected results in some cases. A better approach might be to use explicit server-side preferences (like text/html over application/json) or alphabetic ordering as a deterministic fallback. See also http://www.neeraj.name/2010/11/23/mime-type-resolution-in-rails.html (and especially Erik's comment) for a good discussion of related issues.
        Hide
        Felix Meschberger added a comment -

        Thanks for the link.

        I crosschecked the implementation: It internally uses a TreeSet to manage the quality tagged types. Provided q values are equal for two entries in the Accept header, they are ordered by their MIME type causing application/json to be order before text/html.

        I think this is wrong and should be changed to order by their location in the header.

        While browser might not place specific preference on entries with the same q value (and the spec fails to clarify), I think it is more natural to use the declaration order in the header than it is to use string ordering.

        Show
        Felix Meschberger added a comment - Thanks for the link. I crosschecked the implementation: It internally uses a TreeSet to manage the quality tagged types. Provided q values are equal for two entries in the Accept header, they are ordered by their MIME type causing application/json to be order before text/html. I think this is wrong and should be changed to order by their location in the header. While browser might not place specific preference on entries with the same q value (and the spec fails to clarify), I think it is more natural to use the declaration order in the header than it is to use string ordering.
        Hide
        Felix Meschberger added a comment -

        Proposed patch using declaration order instead of string comparison and fixing the "consistency with equals" situation of the MediaRange.compareTo method.

        Show
        Felix Meschberger added a comment - Proposed patch using declaration order instead of string comparison and fixing the "consistency with equals" situation of the MediaRange.compareTo method.
        Hide
        Bertrand Delacretaz added a comment -

        I much prefer Jukka's proposal to use server-side configured preferences. It's much more predictable than relying on the client ordering of the entries.

        We can simply add an ordered list of preferred content types to the post servlet configuration, and use those to resolve ties.

        Show
        Bertrand Delacretaz added a comment - I much prefer Jukka's proposal to use server-side configured preferences. It's much more predictable than relying on the client ordering of the entries. We can simply add an ordered list of preferred content types to the post servlet configuration, and use those to resolve ties.
        Hide
        Alexander Klimetschek added a comment -

        > use server-side configured preferences

        +1

        Show
        Alexander Klimetschek added a comment - > use server-side configured preferences +1
        Hide
        Carsten Ziegeler added a comment -

        I'm against using server-side configured prefs - assuming that we have well behaving clients (they specify the correct oder), they won't get what they are asking for. Assuming we have clients not well behaving (they don't care about the order), well then they get something and they have to cope with it.
        But adding a server config does not add real value but just adds another config option where people don't know what to configure

        Show
        Carsten Ziegeler added a comment - I'm against using server-side configured prefs - assuming that we have well behaving clients (they specify the correct oder), they won't get what they are asking for. Assuming we have clients not well behaving (they don't care about the order), well then they get something and they have to cope with it. But adding a server config does not add real value but just adds another config option where people don't know what to configure
        Hide
        Bertrand Delacretaz added a comment -

        Thinking again about this, I think the SlingPostServlet is currently doing it wrong: SlingHttpServletRequest.getResponseContentType() should be used to decide which content type to return, so if we want to take the Accept header into consideration that should be done there - probably using a new ResponseMimeTypeSelector service that implements the selection logic in a central place.

        And maybe add a new getResponseContentType(options) where options specifies if the caller wants to consider only the extension, or extension + Accept header using whatever ResponseMimeTypeSelector service is currently active.

        Using the Accept header as per the http spec is cool and useful, but in practice often unusable due to broken or badly configured clients - so I think we must give Sling users the choice of whether and how to use the Accept header.

        Show
        Bertrand Delacretaz added a comment - Thinking again about this, I think the SlingPostServlet is currently doing it wrong: SlingHttpServletRequest.getResponseContentType() should be used to decide which content type to return, so if we want to take the Accept header into consideration that should be done there - probably using a new ResponseMimeTypeSelector service that implements the selection logic in a central place. And maybe add a new getResponseContentType(options) where options specifies if the caller wants to consider only the extension, or extension + Accept header using whatever ResponseMimeTypeSelector service is currently active. Using the Accept header as per the http spec is cool and useful, but in practice often unusable due to broken or badly configured clients - so I think we must give Sling users the choice of whether and how to use the Accept header.
        Hide
        Felix Meschberger added a comment -

        The problem is, that the more we add, the more complex it becomes ...

        Currently it behaves like this:

        If there is a ":http-equiv-accept" request parameter use that as the list of media types accepted/requested
        Otherwise consider the Accept request header

        Then build an internal sorted set from this value. Sort order is defined by the q value and the concreteness of the main and sub types. That is text/html comes before text/*. In a tie the types are string-compared. My patch replaces the string-comparison by a declaration order thing. Which IMHO can be much better defined.

        So, basically, we do not need another mechanism to overwrite this ... the client can overwrite the default Accept header with an :http-equiv-accept request attribute and we are fine IMHO.

        Show
        Felix Meschberger added a comment - The problem is, that the more we add, the more complex it becomes ... Currently it behaves like this: If there is a ":http-equiv-accept" request parameter use that as the list of media types accepted/requested Otherwise consider the Accept request header Then build an internal sorted set from this value. Sort order is defined by the q value and the concreteness of the main and sub types. That is text/html comes before text/*. In a tie the types are string-compared. My patch replaces the string-comparison by a declaration order thing. Which IMHO can be much better defined. So, basically, we do not need another mechanism to overwrite this ... the client can overwrite the default Accept header with an :http-equiv-accept request attribute and we are fine IMHO.
        Hide
        Alexander Klimetschek added a comment -

        @Carsten: I (and Bertrand, IIUC) meant server-side prefs only for conflicts in the accept header (i.e. two mime-types with equal weight), where as per the HTTP spec there is no preference given by the client. So it's perfectly fine to use a server preference in this case. As noted in the link from Jukka, this will make the response stable (even if you consider the request to be "broken" in this case). Currently the handling is also decided by the server - by sorting alphabetically by the mime-type - hence it is stable, but not configurable.

        On the other hand, I guess this is a very specific case, and is in most cases better solved by fixing the client or supporting the simpler extension-based selection of response types.

        Show
        Alexander Klimetschek added a comment - @Carsten: I (and Bertrand, IIUC) meant server-side prefs only for conflicts in the accept header (i.e. two mime-types with equal weight), where as per the HTTP spec there is no preference given by the client. So it's perfectly fine to use a server preference in this case. As noted in the link from Jukka, this will make the response stable (even if you consider the request to be "broken" in this case). Currently the handling is also decided by the server - by sorting alphabetically by the mime-type - hence it is stable, but not configurable. On the other hand, I guess this is a very specific case, and is in most cases better solved by fixing the client or supporting the simpler extension-based selection of response types.
        Hide
        Alexander Klimetschek added a comment -

        > the client can overwrite the default Accept header with an :http-equiv-accept request -attribute- parameter

        Are extensions supported? I think this would be more consistent with Sling's request processing where the extension takes a central role (and the accept header isn't used at all).

        Show
        Alexander Klimetschek added a comment - > the client can overwrite the default Accept header with an :http-equiv-accept request - attribute - parameter Are extensions supported? I think this would be more consistent with Sling's request processing where the extension takes a central role (and the accept header isn't used at all).
        Hide
        Felix Meschberger added a comment -

        > Are extensions supported?

        Not at the moment.

        > SlingHttpServletRequest.getResponseContentType()

        We implement that method by looking at the request extension (getResponseContentTypes() returns an Enumeration just containing getResponseContentType()).

        I think this is a good approach:

        • getResponseContentType() returns a primary requested type by the client:
          1. derived from the request extension (if any)
          2. derived from Accept header (as per today's MediaRangeList)
        • getResponseContentTypes() returns an enumeration containing
          1. type derived from the request extension (if any)
          2. types derived from Accept header (in q and declaration order)
        • getResponseContentType(String.. supported)
          returns the response content type amongst the supported types being returned first by the getResponseContentTypes() enumeration

        Finally the Sling POST Servlet might use the SlingHttpServletResponse.getResponseContentType("text/html", "text/json") call to get the desired type

        Alternatively:

        • keep the SlingHttpServletResponseImpl implementation as it is (and no API extension)
        • create a request filter overwriting the getResponseContentType and getResponseContentTypes() methods along the lines defined above
        • have the Sling POST Servlet check the getResponseContentTypes() enumeration and selecting the first supported content type from the enumeration

        Actually, I would prefer the filter solution because it does not require changes to the API or the engine bundle and is ultimatively flexible. In fact this is a similar solution as we chose for I18N support and the getLocale methods.

        WDYT ?

        Show
        Felix Meschberger added a comment - > Are extensions supported? Not at the moment. > SlingHttpServletRequest.getResponseContentType() We implement that method by looking at the request extension (getResponseContentTypes() returns an Enumeration just containing getResponseContentType()). I think this is a good approach: getResponseContentType() returns a primary requested type by the client: 1. derived from the request extension (if any) 2. derived from Accept header (as per today's MediaRangeList) getResponseContentTypes() returns an enumeration containing 1. type derived from the request extension (if any) 2. types derived from Accept header (in q and declaration order) getResponseContentType(String.. supported) returns the response content type amongst the supported types being returned first by the getResponseContentTypes() enumeration Finally the Sling POST Servlet might use the SlingHttpServletResponse.getResponseContentType("text/html", "text/json") call to get the desired type Alternatively: keep the SlingHttpServletResponseImpl implementation as it is (and no API extension) create a request filter overwriting the getResponseContentType and getResponseContentTypes() methods along the lines defined above have the Sling POST Servlet check the getResponseContentTypes() enumeration and selecting the first supported content type from the enumeration Actually, I would prefer the filter solution because it does not require changes to the API or the engine bundle and is ultimatively flexible. In fact this is a similar solution as we chose for I18N support and the getLocale methods. WDYT ?
        Hide
        Alexander Klimetschek added a comment -

        If we don't want to change the API at this point, we could just leave out adding getResponseContentType(String.. supported) and do this logic inside the SlingPostServlet. A utility method taking both the getResponseContentTypes() enumeration and an ordered list of supported types from the servlet is probably better than putting this into the request API.

        Then you only have to adapt the implementation for getResponseContentType()/getResponseContentTypes() to add 2. (accept header).

        I think we have too many filters already, makes the stack traces so long

        BTW: Is getResponseContentType() / getResponseContentTypes() used for the servlet resolution? (mapping mime-types back to plain extensions)

        Show
        Alexander Klimetschek added a comment - If we don't want to change the API at this point, we could just leave out adding getResponseContentType(String.. supported) and do this logic inside the SlingPostServlet. A utility method taking both the getResponseContentTypes() enumeration and an ordered list of supported types from the servlet is probably better than putting this into the request API. Then you only have to adapt the implementation for getResponseContentType()/getResponseContentTypes() to add 2. (accept header). I think we have too many filters already, makes the stack traces so long BTW: Is getResponseContentType() / getResponseContentTypes() used for the servlet resolution? (mapping mime-types back to plain extensions)
        Hide
        Felix Meschberger added a comment -

        > I think we have too many filters already, makes the stack traces so long

        I am sure, this is a joke

        The reason for the filter is that we are more flexible and I don't want to grow the Engine too much.

        > BTW: Is getResponseContentType() / getResponseContentTypes() used for the servlet resolution?

        No, it is not used. For Mime Type mapping we either use the ServletContext.getMimeType method (which is backed by the MimeTypeService) or use the MimeTypeService directly.

        Show
        Felix Meschberger added a comment - > I think we have too many filters already, makes the stack traces so long I am sure, this is a joke The reason for the filter is that we are more flexible and I don't want to grow the Engine too much. > BTW: Is getResponseContentType() / getResponseContentTypes() used for the servlet resolution? No, it is not used. For Mime Type mapping we either use the ServletContext.getMimeType method (which is backed by the MimeTypeService) or use the MimeTypeService directly.
        Hide
        Alexander Klimetschek added a comment -

        > The reason for the filter is that we are more flexible and I don't want to grow the Engine too much.

        Sling's SlingHttpServletRequest API defines getResponseContentType(), so I wonder why this should be implemented in a filter and not in SlingHttpServletRequestImpl itself (making the current implementation of it useless). Servlet filters IMO only make sense if an application wants to generally wrap things for all servlets, if an applications wants to overwrite servlet-container default logic or have something that is active or not depending on the request.

        In this case however, I think we agree that this is core framework logic. Only if someone wants to change it, she could write a filter to provide a different logic for response content types.

        Show
        Alexander Klimetschek added a comment - > The reason for the filter is that we are more flexible and I don't want to grow the Engine too much. Sling's SlingHttpServletRequest API defines getResponseContentType(), so I wonder why this should be implemented in a filter and not in SlingHttpServletRequestImpl itself (making the current implementation of it useless). Servlet filters IMO only make sense if an application wants to generally wrap things for all servlets, if an applications wants to overwrite servlet-container default logic or have something that is active or not depending on the request. In this case however, I think we agree that this is core framework logic. Only if someone wants to change it, she could write a filter to provide a different logic for response content types.
        Hide
        Carsten Ziegeler added a comment -

        I just reread this issue (in order to prepare the next release) and I fail to see how we should resolve this? Any ideas?

        Show
        Carsten Ziegeler added a comment - I just reread this issue (in order to prepare the next release) and I fail to see how we should resolve this? Any ideas?
        Hide
        Felix Meschberger added a comment -

        Lets postpone to the next release...

        Show
        Felix Meschberger added a comment - Lets postpone to the next release...
        Hide
        Carsten Ziegeler added a comment -

        Moved to next version

        Show
        Carsten Ziegeler added a comment - Moved to next version
        Hide
        Felix Meschberger added a comment -

        Since the order of equally weighted content types is not defined and thus is not deterministic, Sling works correctly and we have nothing to do here IMHO.

        I would prefer to resolve this issue as won't fix.

        Show
        Felix Meschberger added a comment - Since the order of equally weighted content types is not defined and thus is not deterministic, Sling works correctly and we have nothing to do here IMHO. I would prefer to resolve this issue as won't fix.

          People

          • Assignee:
            Unassigned
            Reporter:
            Felix Meschberger
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development