lilinkevin / rest-assured

Automatically exported from code.google.com/p/rest-assured
0 stars 0 forks source link

CharsetExtractor fails to extract charset when more than one key=value pair present #337

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. This is our resource declaration: 
@Produces("application/ld+json;charset=UTF-8;qs=0.5")
2. This is our rest assured code: 
expect().statusCode(200).body(is(notNullValue())).when().get(url);
3. Error is thrown after 0.5 is extracted as the charset.

What is the expected output? What do you see instead?
We expect UTF-8 to be extracted as the charset.

What version of the product are you using? On what operating system?
We are using rest assured version 2.3.1. This occurs on Linux, I expect it to 
be the same on other operating systems.

Please provide any additional information below.
The problem lies in the current implementation of CharsetExtractor. It returns 
the value of the *final* key=value pair, regardless of the key. So therefore
com.jayway.restassured.internal.http.CharsetExtractor.getCharsetFromContentType(
"application/ld+json; charset=UTF-8; qs=0.5") returns 0.5, but
com.jayway.restassured.internal.http.CharsetExtractor.getCharsetFromContentType(
"application/ld+json; qs=0.5; charset=UTF-8") returns UTF-8.

However, changing the order of qs and charset on the @Produces annotation 
doesn't help, as they are always passed in the same order.

The fix could be replacing line 36 of CharsetExtractor with something along the 
lines of
if(questionMark != null && questionMark.length == 2 && 
questionMark[0].trim().equals("charset")) 

Original issue reported on code.google.com by m.huniew...@gmail.com on 4 Jun 2014 at 4:03

GoogleCodeExporter commented 9 years ago
I have created a pull request for this: 
https://github.com/jayway/rest-assured/pull/33

Original comment by m.huniew...@gmail.com on 4 Jun 2014 at 5:24

GoogleCodeExporter commented 9 years ago
Thanks for the pull request, I've merged it now :) Did a small change by using 
equalsIgnoreCase instead of equals. 

Original comment by johan.ha...@gmail.com on 4 Jun 2014 at 5:33

GoogleCodeExporter commented 9 years ago
Btw, you can try it out by depending on version 2.3.2-SNAPSHOT after having 
added the following maven repo:

<repositories>
        <repository>
            <id>sonatype</id>
            <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
            <snapshots />
        </repository>
</repositories>

Original comment by johan.ha...@gmail.com on 4 Jun 2014 at 5:45

GoogleCodeExporter commented 9 years ago
Hi, I tried that and it fixed the problem.

I am not sure about the case - is it allowed to be capital according to the 
documentation? If so, might be worth providing a test case for that scenario.

Original comment by m.huniew...@gmail.com on 4 Jun 2014 at 6:39

GoogleCodeExporter commented 9 years ago
According to RFC 1341 (http://www.w3.org/Protocols/rfc1341/4_Content-Type.html) 
subtypes such as "charset" are case insensitive so I think it's correct now. 
I've already added a test :)

Thanks for trying it out.

Original comment by johan.ha...@gmail.com on 4 Jun 2014 at 6:45

GoogleCodeExporter commented 9 years ago
Sorry that should be "parameter names" and not "subtype" (even though both are 
case insensitive).

Original comment by johan.ha...@gmail.com on 4 Jun 2014 at 6:47

GoogleCodeExporter commented 9 years ago
Cool, nice. I saw the build has failed, but it doesn't seem to be related to my 
change...

Also, when do you think this is coming out (we have a policy of not depending 
on snapshots)?

Original comment by m.huniew...@gmail.com on 4 Jun 2014 at 6:47

GoogleCodeExporter commented 9 years ago
I know, it seems like some tests that goes out to external servers fail on 
Jenkins. I really should look into it but they all seem to work from Sweden :)

I'm actually about to release 2.3.2 soon. Probably tomorrow or on monday.

Original comment by johan.ha...@gmail.com on 4 Jun 2014 at 6:49

GoogleCodeExporter commented 9 years ago
Thanks, Johan! I'll keep en eye on it (we have a non-snapshot policy). :)

Original comment by m.huniew...@gmail.com on 5 Jun 2014 at 9:12

GoogleCodeExporter commented 9 years ago
Hopefully the fix will be released soon, but the workaround is to remove the 
*body* check:

given().request().header("Accept", "application/ld+json") 
.expect().statusCode(200).body(is(notNullValue())) 
        .contentType("application/ld+json").when().get(readServiceUrl + 
testArticleUuid.toString())

becomes

given().request().header("Accept", "application/ld+json") 
.expect().statusCode(200).      .contentType("application/ld+json").when().get(readS
erviceUrl + testArticleUuid.toString())

I believe the body can be read and inspected later.

Original comment by m.huniew...@gmail.com on 5 Jun 2014 at 2:57

GoogleCodeExporter commented 9 years ago
Didn't had time to release today so I'll do it on monday (most likely).

Original comment by johan.ha...@gmail.com on 5 Jun 2014 at 5:17

GoogleCodeExporter commented 9 years ago
Oh, shame, I'm waiting impatiently. ;) Thanks for the update.

NB I managed to read the body in the following manner:
new String(response.get().body().asByteArray(), "UTF-8");

Original comment by m.huniew...@gmail.com on 5 Jun 2014 at 5:21