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
        Shawn Clark added a comment -

        I looked at this a bit further and I realized that the BufferedImageHttpMessageConverter might be using a bad design when extending the AbstractHttpMessageConverter. Within the AbstractHttpMessageConverter there is the supportedMediaTypes list. This would contain a list of MediaType objects that the implementing class would support. BufferedImageHttpMessageConverter never uses the supportedMediaTypes and as such is not using it within its canWrite(Class<?>, MediaType) call.

        If you look at the MappingJacksonHttpMessageConverter as a reference you can see that within the canWrite(Class<?>, MediaType) it does a check to make sure the class is something it can deal with and then calls the canWrite(mediaType) of the AbstractHttpMessageConverter.

        Shouldn't the BufferedImageHttpMessageConverter use the same practice? What would need to be changed is that in the constructor it would put each of the ImageIO writer MIME types into the supportedMediaTypes list.

        BufferedImageHttpMessageConverter()
        	public BufferedImageHttpMessageConverter() {
        		String[] readerMediaTypes = ImageIO.getReaderMIMETypes();
        		for (String mediaType : readerMediaTypes) {
        			this.readableMediaTypes.add(MediaType.parseMediaType(mediaType));
        		}
        
        		String[] writerMimeTypes= ImageIO.getWriterMIMETypes();
        		if (writerMimeTypes.length > 0) {
        			this.defaultContentType = MediaType.parseMediaType(writerMimeTypes[0]);
        		}
        
        		List<MediaType> writerMediaTypes = new ArrayList<MediaType>();
        		for (String mimeType : writerMimeTypes) {
        			try {
        				MediaType mediaType = MediaType.parseMediaType(mimeType);
        				writerMediaTypes.add(mediaType);
        			} catch (IllegalArgumentException e) {
        				// if the mime type can't be parsed into a MediaType then it can't be supported
        			}
        		}
        		setSupportedMediaTypes(writerMediaTypes);
        	}
        

        Once that is done the canWrite(Class<?>, MediaType) method no longer needs to check with the ImageIO with the isWritable(MediaType).

        BufferedImageHttpMessageConverter.canWrite(Class<?>, MediaType)
        	public boolean canWrite(Class<?> clazz, MediaType mediaType) {
        		return (BufferedImage.class.equals(clazz) && canWrite(mediaType));
        	}
        

        There would still need to have the change within the write() method to look for the MediaType.ALL and at that point use the deafultContentType.

        Show
        Shawn Clark added a comment - I looked at this a bit further and I realized that the BufferedImageHttpMessageConverter might be using a bad design when extending the AbstractHttpMessageConverter. Within the AbstractHttpMessageConverter there is the supportedMediaTypes list. This would contain a list of MediaType objects that the implementing class would support. BufferedImageHttpMessageConverter never uses the supportedMediaTypes and as such is not using it within its canWrite(Class<?>, MediaType) call. If you look at the MappingJacksonHttpMessageConverter as a reference you can see that within the canWrite(Class<?>, MediaType) it does a check to make sure the class is something it can deal with and then calls the canWrite(mediaType) of the AbstractHttpMessageConverter. Shouldn't the BufferedImageHttpMessageConverter use the same practice? What would need to be changed is that in the constructor it would put each of the ImageIO writer MIME types into the supportedMediaTypes list. BufferedImageHttpMessageConverter() public BufferedImageHttpMessageConverter() { String [] readerMediaTypes = ImageIO.getReaderMIMETypes(); for ( String mediaType : readerMediaTypes) { this .readableMediaTypes.add(MediaType.parseMediaType(mediaType)); } String [] writerMimeTypes= ImageIO.getWriterMIMETypes(); if (writerMimeTypes.length > 0) { this .defaultContentType = MediaType.parseMediaType(writerMimeTypes[0]); } List<MediaType> writerMediaTypes = new ArrayList<MediaType>(); for ( String mimeType : writerMimeTypes) { try { MediaType mediaType = MediaType.parseMediaType(mimeType); writerMediaTypes.add(mediaType); } catch (IllegalArgumentException e) { // if the mime type can't be parsed into a MediaType then it can't be supported } } setSupportedMediaTypes(writerMediaTypes); } Once that is done the canWrite(Class<?>, MediaType) method no longer needs to check with the ImageIO with the isWritable(MediaType). BufferedImageHttpMessageConverter.canWrite(Class<?>, MediaType) public boolean canWrite( Class <?> clazz, MediaType mediaType) { return (BufferedImage.class.equals(clazz) && canWrite(mediaType)); } There would still need to have the change within the write() method to look for the MediaType.ALL and at that point use the deafultContentType.
        Hide
        Shawn Clark added a comment -

        Unfortunately I didn't look closely enough at the BufferedImageHttpMessageConverter to see that it doesn't extend the AbstractHttpMessageConverter. It just implements the HttpMessageConverter directly. As such my last comment isn't a proper solution but the initial issue is still something that needs to be resolved.

        Show
        Shawn Clark added a comment - Unfortunately I didn't look closely enough at the BufferedImageHttpMessageConverter to see that it doesn't extend the AbstractHttpMessageConverter. It just implements the HttpMessageConverter directly. As such my last comment isn't a proper solution but the initial issue is still something that needs to be resolved.
        Hide
        Shawn Clark added a comment -

        Why doesn't BufferedImageHttpMessageConverter extend the AbstractHttpMessageConverter ? There is the concept of a defaultContentType and it handles the MediaType.ALL properly. Any reason why this class specifically didn't extend it?

        Show
        Shawn Clark added a comment - Why doesn't BufferedImageHttpMessageConverter extend the AbstractHttpMessageConverter ? There is the concept of a defaultContentType and it handles the MediaType.ALL properly. Any reason why this class specifically didn't extend it?
        Hide
        Shawn Clark added a comment -

        Temporary fix if someone is wanting to get the defaultContentType to work a little better. Just replace the original BufferedImageHttpMessageConverter with this class in your application context definition for the AnnotationmethodHandlerAdapter bean.

        EnhancedBufferedImageHttpMessageConverter.class
        public class EnhancedBufferedImageHttpMessageConverter extends BufferedImageHttpMessageConverter {
        	private List<MediaType> writerMediaTypes = new ArrayList<MediaType>();
        
        	public DCEBufferedImageHttpMessageConverter() {
        		String[] writerMimeTypes = ImageIO.getWriterMIMETypes();
        		for (String mimeType : writerMimeTypes) {
        			try {
        				writerMediaTypes.add(MediaType.parseMediaType(mimeType));
        			} catch (IllegalArgumentException e) {
        				// If the mimeType can't be parsed then it can't be a valid writerMediaType so ignore and go onto the next
        			}
        		}
        	}
        
        	@Override
        	public boolean canWrite(Class<?> clazz, MediaType mediaType) {
        		return (BufferedImage.class.equals(clazz) && isWritable(mediaType));
        	}
        
        	private boolean isWritable(MediaType mediaType) {
        		if (mediaType == null) {
        			return true;
        		}
        
        		for (MediaType writerMediaType : writerMediaTypes) {
        			if (writerMediaType.isCompatibleWith(mediaType)) {
        				return true;
        			}
        		}
        
        		return false;
        	}
        
        	@Override
        	public void write(BufferedImage image, MediaType contentType, HttpOutputMessage outputMessage) throws IOException, HttpMessageNotWritableException {
        		MediaType checkedContentType = contentType;
        		if (contentType.isWildcardType() || contentType.isWildcardSubtype()) {
        			checkedContentType = null;
        		}
        		super.write(image, checkedContentType, outputMessage);
        	}
        }
        
        Show
        Shawn Clark added a comment - Temporary fix if someone is wanting to get the defaultContentType to work a little better. Just replace the original BufferedImageHttpMessageConverter with this class in your application context definition for the AnnotationmethodHandlerAdapter bean. EnhancedBufferedImageHttpMessageConverter.class public class EnhancedBufferedImageHttpMessageConverter extends BufferedImageHttpMessageConverter { private List<MediaType> writerMediaTypes = new ArrayList<MediaType>(); public DCEBufferedImageHttpMessageConverter() { String [] writerMimeTypes = ImageIO.getWriterMIMETypes(); for ( String mimeType : writerMimeTypes) { try { writerMediaTypes.add(MediaType.parseMediaType(mimeType)); } catch (IllegalArgumentException e) { // If the mimeType can't be parsed then it can't be a valid writerMediaType so ignore and go onto the next } } } @Override public boolean canWrite( Class <?> clazz, MediaType mediaType) { return (BufferedImage.class.equals(clazz) && isWritable(mediaType)); } private boolean isWritable(MediaType mediaType) { if (mediaType == null ) { return true ; } for (MediaType writerMediaType : writerMediaTypes) { if (writerMediaType.isCompatibleWith(mediaType)) { return true ; } } return false ; } @Override public void write(BufferedImage image, MediaType contentType, HttpOutputMessage outputMessage) throws IOException, HttpMessageNotWritableException { MediaType checkedContentType = contentType; if (contentType.isWildcardType() || contentType.isWildcardSubtype()) { checkedContentType = null ; } super .write(image, checkedContentType, outputMessage); } }
        Hide
        Rick Herrick added a comment -

        I just ran into this bug when trying to run the Spring Security OAuth 2.0 samples under Google Chrome. Chrome uses seems to use Accept: */* as its default media type accept header, which runs straight into this problem. It'd be really nice to implement a fix for this, because it's bit some other people trying to run these sample apps as well. It's fiendishly difficult to ferret out, but could be fixed with an hour worth of work, as shown in the fixes that Shawn Clark suggests above.

        Show
        Rick Herrick added a comment - I just ran into this bug when trying to run the Spring Security OAuth 2.0 samples under Google Chrome. Chrome uses seems to use Accept: */ * as its default media type accept header, which runs straight into this problem. It'd be really nice to implement a fix for this, because it's bit some other people trying to run these sample apps as well. It's fiendishly difficult to ferret out, but could be fixed with an hour worth of work, as shown in the fixes that Shawn Clark suggests above.
        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, 4 days ago