sahaya / rest-assured

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

Extend content type API with : ContentType.JSON.withCharset(...) #228

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
So I have this code with the ContentType.JSON

    expect()
        .statusCode(200)
    .given()
        .contentType(ContentType.JSON)
        .body(ImmutableMap.builder()
            .put("accented_string", "2ème étage à gauche")
            .build())
    .when()
        .post(BASE_URL + "/address/comment");

That is sent with the default encoding ISO-8859-1 to comply with the RFC 2616, 
as mentioned in the wiki or the changelog 
(https://raw.github.com/jayway/rest-assured/master/changelog.txt). However 
while my JAXRS server accept UTF-8 or ISO-8859-1, I experienced encoding issues 
with the passed json payload. Anyway, as the current service should work with 
UTF-8 charset, I looked for ways to specify the charset in the Content-Type 
header.

And the only way to do that is to pass a string with the charset :
        .contentType("application/json; charset=utf-8")

I believe this would be much more user friendly if one could specify the 
charset of the Content-Type per request, using the ContentType enum with a 
charset extension, like that :
        .contentType(ContentType.JSON.withCharset("UTF-8"))

Of course this might lead to some change in the API as it currently takes a 
ContentType enum. Maybe you could add another method 
#contentType(ContentTypeWithCharset contentTypeWithCharset), and add a simple 
factory method that takes the enum value and the charset into a new object that 
implements ContentTypeWithCharset.
        ContentType.JSON.withCharset("UTF-8")

Or why not add another charset(String) method in the fluent 
RequestSpecification.

At the moment we are using version 1.6.2, but given the changelog, version up 
to 1.8.0 don't offer a similar API but with strings.

Thoughts ?

Brice

Original issue reported on code.google.com by brice.du...@gmail.com on 17 Apr 2013 at 9:15

GoogleCodeExporter commented 9 years ago
ContentType.JSON.withCharset("UTF-8") seems like a good idea to me and 
shouldn't be too hard to implement. Perhaps you want to help out with this and 
I'll include it in the next release? It's also possible to change the default 
charset using the configuration (see documentation). 

Regards,
/Johan

Original comment by johan.ha...@gmail.com on 18 Apr 2013 at 6:30

GoogleCodeExporter commented 9 years ago
Hi Johan,

Yes I saw the default configuration before writing this issue, but I wanted to 
have a way to show the charset used per request, as it makes clearer to the 
future reader what charset is used. Plus it made sense to me in the well 
crafted API of RESTAssured.

Not sure I can give it a try soon, with many other side project. I'll see what 
I can do though.

Cheers,
Brice

Original comment by brice.du...@gmail.com on 18 Apr 2013 at 7:53

GoogleCodeExporter commented 9 years ago
No problems. I'll see what I can do!

Original comment by johan.ha...@gmail.com on 18 Apr 2013 at 8:05

GoogleCodeExporter commented 9 years ago
I've implemented this is master now. Please depend on version 1.8.1-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 18 Apr 2013 at 11:00

GoogleCodeExporter commented 9 years ago
wow that was fast :)

Original comment by brice.du...@gmail.com on 18 Apr 2013 at 11:37

GoogleCodeExporter commented 9 years ago
It wasn't that hard :)

Original comment by johan.ha...@gmail.com on 18 Apr 2013 at 11:42

GoogleCodeExporter commented 9 years ago
I just took a look at your code, indeed your modification is simple ; I was 
thinking about a much more type oriented design ;)
Anyhow I feel ashamed now ;)

Cheers,
Brice

Original comment by brice.du...@gmail.com on 18 Apr 2013 at 3:11