The class MediaType has a natural ordering that is inconsistent with equals, which is generally recommended or should otherwise at least be indicated in the javadoc [SPR-6788]

[spring-projects-issues]

Tomas Johansson opened SPR-6788 and commented

For example, if you use these two strings:
"text/html; q=0.7; charset=iso-8859-1"
"text/html; q=0.7"
then the compareTo method will return zero but equals will return false.

I suggest that a consistent implementation should either be fixed, or otherwise (if necessary for some reason ?) at least it should be mentioned in the javadoc that the natural ordering that is inconsistent with equals.

Example of JUnit test that I think should pass:

	@Test
	public void verifyConsistentImplementationOf_equals_And_compareTo() {
		verifyConsistentImplementationOf_equals_And_compareTo(
			MediaType.parseMediaType("text/html; q=0.7; charset=iso-8859-1"),
			MediaType.parseMediaType("text/html; q=0.7")
		);
		verifyConsistentImplementationOf_equals_And_compareTo(
				[add more test cases ...]
		);
		[add more test cases ...]
	}

	private void verifyConsistentImplementationOf_equals_And_compareTo(MediaType mediaType1, MediaType mediaType2) {
		assertEquals(mediaType1.equals(mediaType2), mediaType1.compareTo(mediaType2) == 0);
	}

/ Tomas Johansson

---

Affects: 3.0 GA

[spring-projects-issues]

Arjen Poutsma commented

Added formatting.

[spring-projects-issues]

Arjen Poutsma commented

Thanks for spotting this.

MediaType no longer implements Comparable now. Instead, I've added a sortBySpecificity() method, which sorts a list of media types by specificity.

[spring-projects-issues]

Arjen Poutsma commented

The javadoc of sortBySpecificity() reads:

Given two media types:

1. if either media type has a wildcard type, then the media type without the wildcard is ordered before the other.
2. if the two media types have different types, then they are considered equal and remain their current order.
3. if either media type has a wildcard subtype, then the media type without the wildcard is sorted before the other.
4. if the two media types have different subtypes, then they are considered equal and remain their current order.
5. if the two media types have different quality value, then the media type with the highest quality value is ordered before the other.
6. if the two media types have a different amount of parameters, then the media type with the most parameters is ordered before the other.

For example:

audio/basic < audio/* < */*
audio/* < audio/*;q=0.7; audio/*;q=0.3
audio/basic;level=1 < audio/basic
audio/basic == text/html
audio/basic == audio/wave

[spring-projects-issues]

Tomas Johansson commented

Well, thank you yourself for dealing with the issue within one day, but I must say I was surprised to see that you immediately removed the implementation of the interface, considering the existing client code (among spring framework users) which now will get compiling errors when they have done this:

Collections.sort(mediaTypes);

which now will have to be replaced with this:

MediaType.sortBySpecificity(mediaTypes);

Since Spring is a very much published framework ( see "Public versus Published Interfaces" : http://martinfowler.com/ieeeSoftware/published.pdf ) I think that the javadoc and release notes for 3.0.1 should mention that the interface should be considered as deprecated and in the future will become removed, rather than removing it at once and breaking the client code out there.

Another thing, when I looked in the implementation in the new method 'sortBySpecificity' I noted that it does not really do much itself, except from two things:

* it gives the client an IllegalArgumentException instead of a NullPointerException when the parameter is null (and now I am ignoring the aditional size control which does not seem meaningful since a zero sized list to 'Collections.sort' does not cause any problem except for being a meaningless invocation).
* it enables hiding of the Comparator interface

Regarding the last potential reason for exposing this method into the MediaType class, is it really an appropriate thing to do, as opposed of instead making the Comparator implementation available (either by the MediaType class, or by a new MediaTypeComparatorFactory class) ?

The problem with providing one specific method for that particular sorting is that it can force client programmers to write more clumsy code doing the sorting in different ways, if they want to either use the included Comparator or write their own Comparator.
In other words, there will not be only one way for doing the sorting but the other kind of sortings will have to be treated differently, if someone for example would want to do a reversed sorting, and also would like to do a sorting only considering the quality parameter.
For example some helper method in client code might want to implement something like this:

// example potential client code for providing the options of doing different kind of sortings
public enum MediaTypeSortingStrategy {
	SPECIFICITY, // use the "built-in" Comparator, included in the Spring framework distribution
	SPECIFICITY_REVERSED, // should reverse the "built-in" comparator 
	QUALITY 	 // use our own Comparator     
}

import org.springframework.util.Assert;
public class ... {
/**
 * @throws IllegalArgumentException if mediaTypes is null
 */
public void sortMediaTypes(
	final List<MediaType> mediaTypes, 
	final MediaTypeSortingStrategy mediaTypeSortingStrategy
) {
	// use (i.e. duplicate) the same message as in the built-in method:
	final String assertMessageToBeUsedWhenMediaTypesListIsNull = "'mediaTypes' must not be null";
	
	if(mediaTypeSortingStrategy == MediaTypeSortingStrategy.SPECIFICITY) {
		// now we must use the built-in method, since the Comparator is not publicly available
		MediaType.sortBySpecificity(mediaTypes); // will throw IllegalArgumentException (currently with the same message as above, but of course that might change in a future release...)
	}
	else if(mediaTypeSortingStrategy == MediaTypeSortingStrategy.SPECIFICITY_REVERSED) {
		Assert.notNull(mediaTypes, assertMessageToBeUsedWhenMediaTypesListIsNull); // want the same kind of exception as MediaType.sortBySpecificity will throw
		Collections.sort(
			mediaTypes, 
			new InvertibleComparator<MediaType>(
				// well, actually the code below can not be implemented easily,
				// without copying the Spring framework source code for the comparator
				// and exposing it ourselves into a class as below ...
				MediaTypeComparatorFactory.createSpecifictyComparator() // should return the currently package level protected 'SPECIFICITY_COMPARATOR'
			)
		);
	}		
	else if(mediaTypeSortingStrategy == MediaTypeSortingStrategy.QUALITY) {
		// we do not want a NullPointerException for this scenario but want 
		// a consistent behaviour as when using the "built-in" sorter
		Assert.notNull(mediaTypes, assertMessageToBeUsedWhenMediaTypesListIsNull);
		Collections.sort(mediaTypes, createMediaTypeComparatorSortingByQuality());
	}
	 ...
}
Comparator<MediaType> createMediaTypeComparatorSortingByQuality() { ... } // this method might be located in some factory class

On the other hand, if you (i.e. the Spring framework) would expose the Comparator (and I would also suggest to remove the new method sortBySpecificity), then the above method 'sortMediaTypes' could be implemented by the client code in a more transparent way like below:

public enum MediaTypeSortingStrategy {
	SPECIFICITY, // use the "built-in" Comparator, included in the Spring framework distribution
	SPECIFICITY_REVERSED, // should reverse the "built-in" comparator 
	QUALITY; 	 // use our own Comparator
	
	private static Map<MediaTypeSortingStrategy, Comparator<MediaType>> mediaTypeSortingComparators = new HashMap<MediaTypeSortingStrategy, Comparator<MediaType>>();
	static {
		// Now assume that there is a 'MediaTypeComparatorFactory' in the Spring framework
		// which exposes the Comparator 'SPECIFICITY_COMPARATOR' currently being package level protected within the MediaType class
		mediaTypeSortingComparators.put(SPECIFICITY, MediaTypeComparatorFactory.createSpecifictyComparator());
		mediaTypeSortingComparators.put(SPECIFICITY_REVERSED, new InvertibleComparator<MediaType>(MediaTypeComparatorFactory.createSpecifictyComparator()));

		// The factory name below is ugly, but it should make it obvious that the above factory with Comparator implementation(s) 
		// might be included in Spring framework, while clients might want to write their own Comparators as below  
		mediaTypeSortingComparators.put(QUALITY, ClientCodeMediaTypeComparatorFactory.createMediaTypeComparatorSortingByQuality());
	}
	public Comparator<MediaType> getComparator() {
		return mediaTypeSortingComparators.get(this);
	}
}

/**
 * @throws IllegalArgumentException if mediaTypes is null
 */
public void sortMediaTypes(
	final List<MediaType> mediaTypes, 
	final MediaTypeSortingStrategy mediaTypeSortingStrategy
) {
	// now when the client always defines the error message here, it will always be the same, as opposed 
	// to defining the message here for some of the sortings, while other assertion messages 
	// might be hardcoded in Spring framework itself (which might change and cause different messages)
	final String assertMessageToBeUsedWhenMediaTypesListIsNull = "'mediaTypes' must not be null";
	
	Assert.notNull(mediaTypes, assertMessageToBeUsedWhenMediaTypesListIsNull);
	Collections.sort(mediaTypes, mediaTypeSortingStrategy.getComparator());
}

/ Tomas Johansson

[spring-projects-issues]

Arjen Poutsma commented

Thomas,

There are a couple of good reasons why I removed the Comparable implementation outright, and decided to offer a sortBySpecificity method instead.

Firstly, as you pointed out early in this issue, the original implementation of Comparable was inconsistent with equals and hashCode. The javadoc of Comparable states that this is strongly recommended. So, having a comparator made more sense, also because - as you rightfully comment - sorting by specificity is only of the ways to sort media types. However, if this comparator would have been made public, it allowed usage in sorted sets and maps, which would have caused strange results. The javadoc of Comparator says:

In particular, the sorted set (or sorted map) will violate the general contract for set (or map), which is defined in terms of equals.

(Note that I myself made the mistake of sorting comparable MediaTypes in a TreeMap, in the ContentNegotiatingViewResolver. This could have caused some interesting new JIRAs along the way. This bug was fixed as part of the resolution of this issue.)

I figured that if I could make the mistake of using it in TreeMaps, everybody could, and I wanted to negate that. That's why I decided to offer the sortBySpecificity() method, and not make the comparator public. So, in effect, the specificity comparator is only to be used for sorting lists (and not for TreeMaps and TreeSets).

Some other remarks:

• Throwing a IllegalArgumentException instead of a NullPointerException when null is passed is a standard practice in the Spring Framework
• The > 1 check is there for a reason: sorting a Collections.singletonList() throws an UnsupportedOperationException because the list is unmodifiable. And singleton media types lists are used a lot in Spring. Adding this line here removed 3+ similar size checks in the rest of the code base.
• Finally, you are right to say that removing the Comparable implementation from MediaType breaks backwards compatibility. In retrospect, I think it was a bug to have it in the first place, and that's why it was removed. As public/published interfaces go: I consider MediaType to be an internal class, mostly used in the framework itself, especially when it comes to sorting them. Of course, we will make sure that this will make it to the changelog.

I hope that clears things up a bit, and I hope you understand my reasoning behind these changes.

Cheers,

Arjen

[spring-projects-issues]

Arjen Poutsma commented

After thinking about your comment a bit more, and talking this through with Juergen, we've reinstated the Comparable implementation of MediaType.

It now orders MediaTypes alphabetically, making it consistent with equals.

[spring-projects-issues]

Tomas Johansson commented

Well, that sounded good but it actually is not consistent.
The implementation of Comparable is now based on the value of the toString method, which does not sort.
Example of a failing test with an equals method returning true, while the compareTo does not return zero:

@Test
public void verifyConsistentImplementationOf_equals_And_compareTo() {
     verifyConsistentImplementationOf_equals_And_compareTo(
          "text/html; q=0.7; charset=iso-8859-1",
          "text/html; charset=iso-8859-1; q=0.7"
     );
}
private void verifyConsistentImplementationOf_equals_And_compareTo(String mediaTypeString1, String mediaTypeString2) {
     MediaType mediaType1 = MediaType.parseMediaType(mediaTypeString1);
     MediaType mediaType2 = MediaType.parseMediaType(mediaTypeString2);
     assertEquals(mediaType1.equals(mediaType2), mediaType1.compareTo(mediaType2) == 0);
}

/ Tomas

[spring-projects-issues]

Arjen Poutsma commented

The "text/html; q=0.7; charset=iso-8859-1" vs "text/html; charset=iso-8859-1; q=0.7" inconsistency should now be fixed.

[spring-projects-issues]

Tomas Johansson commented

No, unfortunately not :-(
For example, these two parse strings do not cause a consistent result:
"text/html; q=0.7; charset=iso-8859-1",
"text/html; Q=0.7; charset=iso-8859-1"
They are equals according to the equals method, but compareTo does not return zero.
This particular problem might be resolved by a more consistent treatment of the casing, but would it not also be generally better to reuse code for the equals and compareTo methods, e.g. letting the last part of the equals method implementation return 'compareTo(other) == 0' (or put the code in a private helper method which is reused from both equals and compareTo methods) ?

/ Tomas