From 7e0b013cef37c8353bcb29a25adf74721ba0afef Mon Sep 17 00:00:00 2001 From: Klaus Straubinger Date: Thu, 25 Sep 2014 13:45:39 +0200 Subject: [PATCH] better handling of parameters in server content negotiation Change-Id: Ie2771940b3afea6efb3dc84f8322bc60069052d1 Signed-off-by: Christian Amend --- .../olingo/commons/api/format/AcceptType.java | 18 ++++-- .../olingo/server/core/ContentNegotiator.java | 58 +++++++++---------- .../server/core/ContentNegotiatorTest.java | 43 +++++++++----- 3 files changed, 70 insertions(+), 49 deletions(-) diff --git a/lib/commons-api/src/main/java/org/apache/olingo/commons/api/format/AcceptType.java b/lib/commons-api/src/main/java/org/apache/olingo/commons/api/format/AcceptType.java index 379eb9555..2ac9a278b 100644 --- a/lib/commons-api/src/main/java/org/apache/olingo/commons/api/format/AcceptType.java +++ b/lib/commons-api/src/main/java/org/apache/olingo/commons/api/format/AcceptType.java @@ -27,7 +27,7 @@ import java.util.TreeMap; /** * Internally used {@link AcceptType} for OData library. - * + * * See RFC 7231, chapter 5.3.2: * 
  * Accept = #( media-range [ accept-params ] )
@@ -41,7 +41,7 @@ import java.util.TreeMap;
  * qvalue = ( "0" [ "." 0*3DIGIT ] ) / ( "1" [ "." 0*3("0") ] )
  *
* - * Once created a {@link AcceptType} is IMMUTABLE. + * Once created an {@link AcceptType} is IMMUTABLE. */ public class AcceptType { @@ -82,7 +82,7 @@ public class AcceptType { if (TypeUtil.MEDIA_TYPE_WILDCARD.equals(this.type) && !TypeUtil.MEDIA_TYPE_WILDCARD.equals(subtype)) { throw new IllegalArgumentException("Illegal combination of WILDCARD type with NONE WILDCARD subtype."); } - + final String q = parameters.get(TypeUtil.PARAMETER_Q); if (q == null) { quality = 1F; @@ -124,7 +124,7 @@ public class AcceptType { } /** - * Create a list of {@link AcceptType} objects based on given input string (format). + * Creates a list of {@link AcceptType} objects based on given input string (format). * @param format accept types, comma-separated, as specified for the HTTP header Accept * @return a list of AcceptType objects * @throws IllegalArgumentException if input string is not parseable @@ -142,6 +142,16 @@ public class AcceptType { return result; } + /** + * Creates a list of {@link AcceptType} objects based on given content type. + * @param contentType the content type + * @return an immutable one-element list of AcceptType objects that matches only the given content type + */ + public static List fromContentType(final ContentType contentType) { + return Collections.singletonList(new AcceptType( + contentType.getType(), contentType.getSubtype(), contentType.getParameters(), 1F)); + } + public String getType() { return type; } diff --git a/lib/server-core/src/main/java/org/apache/olingo/server/core/ContentNegotiator.java b/lib/server-core/src/main/java/org/apache/olingo/server/core/ContentNegotiator.java index 1adabf70d..c6fb1bf9b 100644 --- a/lib/server-core/src/main/java/org/apache/olingo/server/core/ContentNegotiator.java +++ b/lib/server-core/src/main/java/org/apache/olingo/server/core/ContentNegotiator.java @@ -36,8 +36,7 @@ public class ContentNegotiator { private ContentNegotiator() {} - private static List - getDefaultSupportedContentTypes(final Class processorClass) { + private static List getDefaultSupportedContentTypes(final Class processorClass) { List defaults = new ArrayList(); if (processorClass == MetadataProcessor.class) { @@ -75,35 +74,23 @@ public class ContentNegotiator { ODataFormat.JSON.name().equalsIgnoreCase(formatString) ? ODataFormat.JSON : ODataFormat.XML.name().equalsIgnoreCase(formatString) ? ODataFormat.XML : ODataFormat.ATOM.name().equalsIgnoreCase(formatString) ? ODataFormat.ATOM : null; - result = getSupportedContentType(format == null ? - ContentType.create(formatOption.getFormat()) : format.getContentType(ODataServiceVersion.V40), - supportedContentTypes); + try { + result = getAcceptedType( + AcceptType.fromContentType(format == null ? + ContentType.create(formatOption.getFormat()) : format.getContentType(ODataServiceVersion.V40)), + supportedContentTypes); + } catch (final IllegalArgumentException e) {} if (result == null) { throw new ContentNegotiatorException("Unsupported $format = " + formatString, ContentNegotiatorException.MessageKeys.UNSUPPORTED_FORMAT_OPTION, formatString); } } else if (acceptHeaderValue != null) { final List acceptedContentTypes = AcceptType.create(acceptHeaderValue); - for (AcceptType acceptedType : acceptedContentTypes) { - for (final ContentType supportedContentType : supportedContentTypes) { - ContentType contentType = supportedContentType; - if (acceptedType.getParameters().containsKey("charset")) { - final String value = acceptedType.getParameters().get("charset"); - if ("utf8".equalsIgnoreCase(value) || "utf-8".equalsIgnoreCase(value)) { - contentType = ContentType.create(contentType, ContentType.PARAMETER_CHARSET_UTF8); - } else { - throw new ContentNegotiatorException("charset in accept header not supported: " + acceptHeaderValue, - ContentNegotiatorException.MessageKeys.WRONG_CHARSET_IN_HEADER, HttpHeader.ACCEPT, acceptHeaderValue); - } - } - if (acceptedType.matches(contentType)) { - result = contentType; - break; - } - } - if (result != null) { - break; - } + try { + result = getAcceptedType(acceptedContentTypes, supportedContentTypes); + } catch (final IllegalArgumentException e) { + throw new ContentNegotiatorException("charset in accept header not supported: " + acceptHeaderValue, e, + ContentNegotiatorException.MessageKeys.WRONG_CHARSET_IN_HEADER, HttpHeader.ACCEPT, acceptHeaderValue); } if (result == null) { throw new ContentNegotiatorException( @@ -114,7 +101,7 @@ public class ContentNegotiator { final ContentType requestedContentType = processorClass == MetadataProcessor.class ? ODataFormat.XML.getContentType(ODataServiceVersion.V40) : ODataFormat.JSON.getContentType(ODataServiceVersion.V40); - result = getSupportedContentType(requestedContentType, supportedContentTypes); + result = getAcceptedType(AcceptType.fromContentType(requestedContentType), supportedContentTypes); if (result == null) { throw new ContentNegotiatorException( "unsupported accept content type: " + requestedContentType + " != " + supportedContentTypes, @@ -125,11 +112,22 @@ public class ContentNegotiator { return result; } - private static ContentType getSupportedContentType(final ContentType requestedContentType, + private static ContentType getAcceptedType(final List acceptedContentTypes, final List supportedContentTypes) { - for (final ContentType supportedContentType : supportedContentTypes) { - if (requestedContentType.isCompatible(supportedContentType)) { - return supportedContentType; + for (final AcceptType acceptedType : acceptedContentTypes) { + for (final ContentType supportedContentType : supportedContentTypes) { + ContentType contentType = supportedContentType; + if (acceptedType.getParameters().containsKey("charset")) { + final String value = acceptedType.getParameters().get("charset"); + if ("utf8".equalsIgnoreCase(value) || "utf-8".equalsIgnoreCase(value)) { + contentType = ContentType.create(contentType, ContentType.PARAMETER_CHARSET_UTF8); + } else { + throw new IllegalArgumentException("charset not supported: " + acceptedType); + } + } + if (acceptedType.matches(contentType)) { + return contentType; + } } } return null; diff --git a/lib/server-core/src/test/java/org/apache/olingo/server/core/ContentNegotiatorTest.java b/lib/server-core/src/test/java/org/apache/olingo/server/core/ContentNegotiatorTest.java index 61d0bc85e..017a098d4 100644 --- a/lib/server-core/src/test/java/org/apache/olingo/server/core/ContentNegotiatorTest.java +++ b/lib/server-core/src/test/java/org/apache/olingo/server/core/ContentNegotiatorTest.java @@ -49,6 +49,7 @@ public class ContentNegotiatorTest { static final private String ACCEPT_CASE_MIN = "application/json;odata.metadata=minimal"; static final private String ACCEPT_CASE_MIN_UTF8 = "application/json;charset=UTF-8;odata.metadata=minimal"; static final private String ACCEPT_CASE_FULL = "application/json;odata.metadata=full"; + static final private String ACCEPT_CASE_NONE = "application/json;odata.metadata=none"; static final private String ACCEPT_CASE_JSONQ = "application/json;q=0.2"; static final private String ACCEPT_CASE_XML = "application/xml"; static final private String ACCEPT_CASE_WILDCARD1 = "*/*"; @@ -62,6 +63,7 @@ public class ContentNegotiatorTest { { ACCEPT_CASE_MIN, null, null, null }, { ACCEPT_CASE_MIN, "json", null, null }, { ACCEPT_CASE_MIN, "json", ACCEPT_CASE_JSONQ, null }, + { ACCEPT_CASE_NONE, ACCEPT_CASE_NONE, null, null }, { "a/a", "a/a", null, "a/a" }, { ACCEPT_CASE_MIN, null, ACCEPT_CASE_JSONQ, null }, { ACCEPT_CASE_MIN, null, ACCEPT_CASE_WILDCARD1, null }, @@ -69,9 +71,11 @@ public class ContentNegotiatorTest { { "a/a", "a/a", null, "a/a,b/b" }, { "a/a", " a/a ", null, " a/a , b/b " }, { "a/a;x=y", "a/a", ACCEPT_CASE_WILDCARD1, "a/a;x=y" }, + { "a/a;v=w;x=y", null, "a/a;x=y", "a/a;b=c,a/a;v=w;x=y" }, + { "a/a;v=w;x=y", "a/a;x=y", null, "a/a;b=c,a/a;v=w;x=y" }, { ACCEPT_CASE_MIN, "json", ACCEPT_CASE_MIN, null }, { ACCEPT_CASE_FULL, null, ACCEPT_CASE_FULL, ACCEPT_CASE_FULL }, - { ACCEPT_CASE_MIN_UTF8, null, ACCEPT_CASE_MIN_UTF8, null }, + { ACCEPT_CASE_MIN_UTF8, null, ACCEPT_CASE_MIN_UTF8, null } }; String[][] casesMetadata = { @@ -85,24 +89,28 @@ public class ContentNegotiatorTest { { ACCEPT_CASE_XML, null, ACCEPT_CASE_WILDCARD2, null }, { "a/a", "a/a", null, "a/a,b/b" }, { "a/a", " a/a ", null, " a/a , b/b " }, - { "a/a;x=y", "a/a", ACCEPT_CASE_WILDCARD1, "a/a;x=y" }, + { "a/a;x=y", "a/a", ACCEPT_CASE_WILDCARD1, "a/a;x=y" } }; String[][] casesFail = { /* expected $format accept additional content types */ - { ACCEPT_CASE_XML, "xxx/yyy", null, null }, - { "a/a", "a/a", null, "b/b" }, - { ACCEPT_CASE_XML, null, ACCEPT_CASE_JSONQ, null }, - { "application/json", null, ACCEPT_CASE_FULL, null }, // not yet supported + { null, "xxx/yyy", null, null }, + { null, "a/a", null, "b/b" }, + { null, "a/a;x=y", null, "a/a;v=w" }, + { null, null, "a/a;x=y", "a/a;v=w" }, + { null, "atom", null, null }, // not yet supported + { null, null, ACCEPT_CASE_FULL, null }, // not yet supported + { null, "a/b;charset=ISO-8859-1", null, "a/b" }, + { null, null, "a/b;charset=ISO-8859-1", "a/b" } }; //CHECKSTYLE:ON //@formatter:on @Test public void testServiceDocumentSingleCase() throws Exception { - String[] useCase = { ACCEPT_CASE_MIN_UTF8, null, ACCEPT_CASE_MIN_UTF8, null }; - - testContentNegotiation(useCase, ServiceDocumentProcessor.class); + testContentNegotiation( + new String[] { ACCEPT_CASE_MIN_UTF8, null, ACCEPT_CASE_MIN_UTF8, null }, + ServiceDocumentProcessor.class); } @Test @@ -114,9 +122,12 @@ public class ContentNegotiatorTest { @Test public void testMetadataSingleCase() throws Exception { - String[] useCase = { ACCEPT_CASE_XML, null, null, null }; + testContentNegotiation(new String[] { ACCEPT_CASE_XML, null, null, null }, MetadataProcessor.class); + } - testContentNegotiation(useCase, MetadataProcessor.class); + @Test(expected = ContentNegotiatorException.class) + public void testMetadataJsonFail() throws Exception { + testContentNegotiation(new String[] { null, "json", null, null }, MetadataProcessor.class); } @Test @@ -127,11 +138,11 @@ public class ContentNegotiatorTest { } @Test - public void testMetadataFail() throws Exception { + public void testEntityCollectionFail() throws Exception { for (String[] useCase : casesFail) { try { - testContentNegotiation(useCase, MetadataProcessor.class); - fail("Exception expected!"); + testContentNegotiation(useCase, EntityCollectionProcessor.class); + fail("Exception expected for '" + useCase[1] + '|' + useCase[2] + '|' + useCase[3] + "'!"); } catch (final ContentNegotiatorException e) {} } } @@ -157,7 +168,9 @@ public class ContentNegotiatorTest { final ContentType requestedContentType = ContentNegotiator.doContentNegotiation(fo, request, p, processorClass); assertNotNull(requestedContentType); - assertEquals(ContentType.create(useCase[0]), requestedContentType); + if (useCase[0] != null) { + assertEquals(ContentType.create(useCase[0]), requestedContentType); + } } private List createCustomContentTypes(final String contentTypeString) {