Spring Framework
  1. Spring Framework
  2. SPR-7763

BufferedImageHttpMessageConverter not using defaultContentType

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Complete
    • Affects Version/s: 3.0.5, 3.1.1
    • Fix Version/s: 3.2.1
    • Component/s: Web
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      Looking at the AnnotationMethodHandler.writeWithmessageConverters() (line: 972). The code loops through each of the acceptedMediaTypes asking the messageConverter if it canWrite(). Using an example acceptedMediaTypes from Firefox would look like:

      [text/html, application/xhtml+xml, application/xml;q=0.9, */*;q=0.8]
      

      Each of the mediaTypes would be passed to the canWrite() and returned as false as it isn't within:

      [image/png, image/jpeg, image/x-png, image/vnd.wap.wbmp, image/bmp, image/gif]
      

      Which is returned by the ImageIO.getWriterMIMETypes().

      Now looking at the BufferedImageHttpMessageConverter there is the ability to specify the defaultContentType to use when no contentType is provided to the write() method. Unfortunately this method never gets called with the MediaType as null. To improve the BufferedImage converter it would make sense to have the isWriteable(MediaType) check for MediaType.ALL. Then within the write() method of the BufferedImageHttpMessageConverter it would use the defaultContentType when passed in a MediaType.ALL.

      BufferedImageHttpMessageConverter.isWriteable()
      	private boolean isWritable(MediaType mediaType) {
      		if (mediaType == null || MediaType.ALL.equals(mediaType)) {
      			return true;
      		}
      		Iterator<ImageWriter> imageWriters = ImageIO.getImageWritersByMIMEType(mediaType.toString());
      		return imageWriters.hasNext();
      	}
      

      This method looks very similar to the AbstractHttpMessageConverter.canWrite(MediaType) except that it compares the mediaType to the ImageIO available MIME types instead of the supportedMediaTypes.

      BufferedImageHttpMessageConverter.write(BufferedImage, MediaType, HttpOutputMessage)
      	public void write(BufferedImage image, MediaType contentType, HttpOutputMessage outputMessage)
      			throws IOException, HttpMessageNotWritableException {
      
      		if (contentType == null || MediaType.ALL.equals(contentType)) {
      			contentType = getDefaultContentType();
      		}
      		
      		...
      	}
      

      Sorry for not having the source code checked out to be able to provide a patch file.

        Activity

        Hide
        Rossen Stoyanchev added a comment -

        I can't look at this in detail at the moment but I just wanted to mention that there have been some significant improvements to content negotiation in the new @MVC support classes in Spring 3.1 specifically in the way we perform matches when a more general media type is requested. For example when "*/*" is requested that's pretty much a request for anything and should match but didn't in 3.0.x. There is also the produces condition that may be useful.

        It would be very helpful if you tried your case with Spring 3.1 making sure that you're using the new support classes. Keep in mind also that the new support classes are intended as a replacement for the old ones and we don't plan to make any further improvements to them, only bug fixes.

        If you still see issues, consider adding a project for the issue at this repository.

        Show
        Rossen Stoyanchev added a comment - I can't look at this in detail at the moment but I just wanted to mention that there have been some significant improvements to content negotiation in the new @MVC support classes in Spring 3.1 specifically in the way we perform matches when a more general media type is requested. For example when "*/*" is requested that's pretty much a request for anything and should match but didn't in 3.0.x. There is also the produces condition that may be useful. It would be very helpful if you tried your case with Spring 3.1 making sure that you're using the new support classes. Keep in mind also that the new support classes are intended as a replacement for the old ones and we don't plan to make any further improvements to them, only bug fixes. If you still see issues, consider adding a project for the issue at this repository .
        Hide
        Rick Herrick added a comment -

        Rossen,

        This is actually happening in Spring 3.1.0.RELEASE. Specifically the private isWritable() call in BufferedImageHttpMessageConverter does this:

        BufferedImageHttpMessageConverter.isWritable()
        private boolean isWritable(MediaType mediaType) {
            if (mediaType == null) {
                return true;
            }
            Iterator<ImageWriter> imageWriters = ImageIO.getImageWritersByMIMEType(mediaType.toString());
            return imageWriters.hasNext();
        }

        When mediaType is /, as in the case of Chrome or Safari with an Accept header of /, ImageIO.getImageWritersByMIMEType() returns an empty iterator, so the *hasNext() method returns false, canWrite() returns false, and AnnotationMethodHandlerAdapter thinks there are no handlers for the image type since there's not a SPECIFIC (i.e. non-wild-card) match for the image type.

        It's possible that we could completely rewrite our MVC support to work around one bug, but it would be better if this means of handling things just worked properly.

        Show
        Rick Herrick added a comment - Rossen, This is actually happening in Spring 3.1.0.RELEASE. Specifically the private isWritable() call in BufferedImageHttpMessageConverter does this: BufferedImageHttpMessageConverter.isWritable() private boolean isWritable(MediaType mediaType) { if (mediaType == null ) { return true ; } Iterator<ImageWriter> imageWriters = ImageIO.getImageWritersByMIMEType(mediaType.toString()); return imageWriters.hasNext(); } When mediaType is / , as in the case of Chrome or Safari with an Accept header of / , ImageIO.getImageWritersByMIMEType() returns an empty iterator, so the *hasNext() method returns false, canWrite() returns false, and AnnotationMethodHandlerAdapter thinks there are no handlers for the image type since there's not a SPECIFIC (i.e. non-wild-card) match for the image type. It's possible that we could completely rewrite our MVC support to work around one bug, but it would be better if this means of handling things just worked properly.
        Hide
        Rossen Stoyanchev added a comment - - edited

        This is actually happening in Spring 3.1.0.RELEASE.

        The issue is marked 3.0.5. Besides that the descriptions clearly refers to AnnotationMethodHandler. When I suggested you try it in 3.1, I also wrote "make sure that you're using the new support classes". That means RequestMappingHandlerAdapter and related classes (not AnnotationMethodHandlerAdapter). Those classes are new in Spring 3.1 and if you follow the links I provided you'll find out more about it.

        It's possible that we could completely rewrite our MVC support to work around one bug, but it would be better if this means of handling things just worked properly.

        We didn't re-write the MVC support classes to work around a specific bug. We did it for more fundamental reasons of customizability and flexibility. In the process we also introduced support for consumes & produces conditions following JAX-RS semantics and did major improvements to the way content negotiation is done. All this was done in the new support classes and we don't intend to continue implementing improvements in both places. That's all I was trying to say. This issue by the way is marked as an improvement.

        Show
        Rossen Stoyanchev added a comment - - edited This is actually happening in Spring 3.1.0.RELEASE. The issue is marked 3.0.5. Besides that the descriptions clearly refers to AnnotationMethodHandler. When I suggested you try it in 3.1, I also wrote "make sure that you're using the new support classes". That means RequestMappingHandlerAdapter and related classes (not AnnotationMethodHandlerAdapter). Those classes are new in Spring 3.1 and if you follow the links I provided you'll find out more about it. It's possible that we could completely rewrite our MVC support to work around one bug, but it would be better if this means of handling things just worked properly. We didn't re-write the MVC support classes to work around a specific bug. We did it for more fundamental reasons of customizability and flexibility. In the process we also introduced support for consumes & produces conditions following JAX-RS semantics and did major improvements to the way content negotiation is done. All this was done in the new support classes and we don't intend to continue implementing improvements in both places. That's all I was trying to say. This issue by the way is marked as an improvement.
        Hide
        Rick Herrick added a comment -

        Yes, the issue is marked 3.0.5. My point is that it still persists in 3.1.0.RELEASE. If I could edit the defect report to update it so that it would reflect it, I would. I would also mark it as not an improvement, but as a bug. Because it is. A bug in 3.1.0.RELEASE.

        Yes, I could follow your links and find out how to rework my MVC configuration. But that's my point: I have to rework my configuration to work around a bug in an existing class. There's plenty of code out there, including the newest Spring Security OAuth samples, that use the old code and there's no way to know, before even running the code, that you need to migrate your configuration. Not everyone can migrate from 3.0.x to 3.1.x just to fix one small BUG. And until AnnotationMethodHandlerAdapter is deprecated, it should work properly.

        Finally, you yourself said: "We don't plan to make any further improvements to them, only bug fixes." Great. How about you fix this bug?

        Show
        Rick Herrick added a comment - Yes, the issue is marked 3.0.5. My point is that it still persists in 3.1.0.RELEASE. If I could edit the defect report to update it so that it would reflect it, I would. I would also mark it as not an improvement, but as a bug. Because it is. A bug in 3.1.0.RELEASE. Yes, I could follow your links and find out how to rework my MVC configuration. But that's my point: I have to rework my configuration to work around a bug in an existing class. There's plenty of code out there, including the newest Spring Security OAuth samples, that use the old code and there's no way to know, before even running the code, that you need to migrate your configuration. Not everyone can migrate from 3.0.x to 3.1.x just to fix one small BUG. And until AnnotationMethodHandlerAdapter is deprecated, it should work properly. Finally, you yourself said: "We don't plan to make any further improvements to them, only bug fixes." Great. How about you fix this bug?
        Hide
        Dave Syer added a comment -

        I think what Rossen is saying is that anyone who is using 3.1 shouldn't need any code changes in Spring - just make sure to use the default handler mapping - so it might be a bug, but it's not one that needs urgent attention in 3.1. AnnotationMethodHandlerAdapter is the problem and it's basically deprecated in favour of RequestMappingHandlerAdapter. I tried it in the sample app that you were using and it worked (see SECOAUTH-219).

        Show
        Dave Syer added a comment - I think what Rossen is saying is that anyone who is using 3.1 shouldn't need any code changes in Spring - just make sure to use the default handler mapping - so it might be a bug, but it's not one that needs urgent attention in 3.1. AnnotationMethodHandlerAdapter is the problem and it's basically deprecated in favour of RequestMappingHandlerAdapter. I tried it in the sample app that you were using and it worked (see SECOAUTH-219 ).

          People

          • Assignee:
            Rossen Stoyanchev
            Reporter:
            Shawn Clark
            Last updater:
            Rossen Stoyanchev
          • Votes:
            3 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              2 years, 6 weeks, 6 days ago