Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RESTEasy Reactive broken text on Windows #19535

Closed
jsmrcka opened this issue Aug 20, 2021 · 6 comments · Fixed by #19602
Closed

RESTEasy Reactive broken text on Windows #19535

jsmrcka opened this issue Aug 20, 2021 · 6 comments · Fixed by #19602
Assignees
Labels
area/rest env/windows Impacts Windows machines kind/bug Something isn't working
Milestone

Comments

@jsmrcka
Copy link
Contributor

jsmrcka commented Aug 20, 2021

Describe the bug

In a Windows environment, RESTEasy Reactive endpoints do not preserve text charset between request and response.

@Path("/echo")
public class EchoResource {

    @POST
    @Path("/text")
    @Consumes(MediaType.TEXT_PLAIN)
    @Produces(MediaType.TEXT_PLAIN)
    public String echo(String request) {
        return request;
    }
}
curl -d "ěščřžýáíé" -H "Content-Type: text/plain; charset=cp1252" http://localhost:8080/echo/text

Response:

e�cr�����

The same goes for an endpoint accepting multipart data (see attached reproducer).

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

resteasy-reactive-encoding-reproducer.zip

unzip ./resteasy-reactive-encoding-reproducer.zip
cd ./resteasy-reactive-encoding-reproducer
./mvnw.cmd clean verify

Output of uname -a or ver

No response

Output of java -version

openjdk version "11.0.11" 2021-04-20 OpenJDK Runtime Environment GraalVM CE 21.1.0 (build 11.0.11+8-jvmci-21.1-b05) OpenJDK 64-Bit Server VM GraalVM CE 21.1.0 (build 11.0.11+8-jvmci-21.1-b05, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.1.3.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.1 (05c21c65bdfed0f71a2f2ada8b84da59348c4c5d) Maven home: C:\Users\hudson.m2\wrapper\dists\apache-maven-3.8.1-bin\2l5mhf2pq2clrde7f7qp1rdt5m\apache-maven-3.8.1 Java version: 11.0.11, vendor: GraalVM Community, runtime: C:\Program Files\GraalVM\graalvm-ce-java11-21.1.0 Default locale: en_US, platform encoding: Cp1252 OS name: "windows server 2019", version: "10.0", arch: "amd64", family: "windows"

Additional information

No response

@jsmrcka jsmrcka added the kind/bug Something isn't working label Aug 20, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 20, 2021

/cc @FroMage, @geoand, @stuartwdouglas

@quarkus-bot quarkus-bot bot added the env/windows Impacts Windows machines label Aug 20, 2021
jsmrcka added a commit to jsmrcka/quarkus-test-suite that referenced this issue Aug 20, 2021
Add `http/jaxrs-reactive`, which is a RESTEasy reactive equivalent of
`http/jaxrs`.
Notable differenes and limitations:
- In a multipart POJO, Java type for `image` and `octet-stream` were
  changed from `InputStream` to `File` - `InputStream` is not supported
  by `resteasy-reactive`.
- Test of a functionality provided by the
  `quarkus.resteasy.multipart.input-part.default-charset` property has
  been disabled for now - it is unsupported in `resteasy-reactive`.
  quarkusio/quarkus#19527
- [FIXED] Possible bug in `resteasy-reactive` - endpoints  consuming
  `MULTIPART_FORM_DATA` cause build failure.
  quarkusio/quarkus#19404.
- Tests dealing with `text/plain` request/response have been disabled
  on Windows due to a bug.
  quarkusio/quarkus#19535
jsmrcka added a commit to jsmrcka/quarkus-test-suite that referenced this issue Aug 20, 2021
Add `http/jaxrs-reactive`, which is a RESTEasy reactive equivalent of
`http/jaxrs`.
Notable differences and limitations:
- In a multipart POJO, Java type for `image` and `octet-stream` were
  changed from `InputStream` to `File` - `InputStream` is not supported
  by `resteasy-reactive`.
- Test of a functionality provided by the
  `quarkus.resteasy.multipart.input-part.default-charset` property has
  been disabled for now - it is unsupported in `resteasy-reactive`.
  quarkusio/quarkus#19527
- [FIXED] Possible bug in `resteasy-reactive` - endpoints  consuming
  `MULTIPART_FORM_DATA` cause build failure.
  quarkusio/quarkus#19404.
- Tests dealing with `text/plain` request/response have been disabled
  on Windows due to a bug.
  quarkusio/quarkus#19535
jsmrcka added a commit to jsmrcka/quarkus-test-suite that referenced this issue Aug 20, 2021
Add `http/jaxrs-reactive`, which is a RESTEasy reactive equivalent of
`http/jaxrs`.
Notable differences and limitations:
- In a multipart POJO, Java type for `image` and `octet-stream` were
  changed from `InputStream` to `File` - `InputStream` is not supported
  by `resteasy-reactive`.
- Test of a functionality provided by the
  `quarkus.resteasy.multipart.input-part.default-charset` property has
  been disabled for now - it is unsupported in `resteasy-reactive`.
  quarkusio/quarkus#19527
- [FIXED] Possible bug in `resteasy-reactive` - endpoints  consuming
  `MULTIPART_FORM_DATA` cause build failure.
  quarkusio/quarkus#19404.
- Tests dealing with `text/plain` request/response have been disabled
  on Windows due to a bug.
  quarkusio/quarkus#19535
@stuartwdouglas
Copy link
Member

This is actually a Restassured issue, if you extract the body as a byte array and compare the bytes the response is correct. For some reason Restassured is not respecting the content type when converting it to a string.

@stuartwdouglas stuartwdouglas added the triage/invalid This doesn't seem right label Aug 23, 2021
@jsmrcka
Copy link
Contributor Author

jsmrcka commented Aug 23, 2021

@stuartwdouglas

I think the issue is that RESTEasy Reactive is not sending charset info in the response content type.
Using the attached reproducer, I can do:

curl -v -d 'test' -H 'Content-Type: text/plain' http://localhost:8080/echo/text
*   Trying ::1:8080...
* connect to ::1 port 8080 failed: Connection refused
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> POST /echo/text HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.71.1
> Accept: */*
> Content-Type: text/plain
> Content-Length: 4
> 
* upload completely sent off: 4 out of 4 bytes
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< content-length: 4
< Content-Type: text/plain
< 
* Connection #0 to host localhost left intact
test

Compare it to the same application modified to use classic RESTEasy (notice the charset in the response content type):

curl -v -d 'test' -H 'Content-Type: text/plain' http://localhost:8080/echo/text
*   Trying ::1:8080...
* connect to ::1 port 8080 failed: Connection refused
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> POST /echo/text HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.71.1
> Accept: */*
> Content-Type: text/plain
> Content-Length: 4
> 
* upload completely sent off: 4 out of 4 bytes
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Length: 4
< Content-Type: text/plain;charset=UTF-8
< 
* Connection #0 to host localhost left intact
test

REST Assured probably uses some kind of default when it does not receive the charset in the response.
Sure, I can modify our tests to explicitly define expected response charset, but that defeats the purpose: we want to verify that a response contains all the necessary information so that a client is able to parse it without extra knowledge and without relying on defaults.

I think we should either properly document (https://quarkus.io/guides/resteasy-reactive) the need to manually specify the charset:

    ...
    @Produces(MediaType.TEXT_PLAIN + ";charset=UTF-8")
    public String echo(String request) {
    ...

or make resteasy-reactive do it automatically, just like resteasy does it.

@stuartwdouglas
Copy link
Member

I swear when I looked in wireshark there was a content type in the response, although this may have been after I made some changes (I added the charset manually to the producer method).

@geoand it looks like we don't default to UTF-8 and RESTEasy does (I think it is configurable). We should probably look at adding this.

@quarkus-bot quarkus-bot bot removed the triage/invalid This doesn't seem right label Aug 23, 2021
@geoand
Copy link
Contributor

geoand commented Aug 24, 2021

I'll have a look

@geoand
Copy link
Contributor

geoand commented Aug 24, 2021

I don't see any configuration in Quarkus that controls this for RESTEasy. Futhermore after testing around a bit, I see that RESTEasy only adds the UTF-8 charset when the response is text/plain.
So I'll do the simplest thing for now, which is just to turn text/plain into text/plain;charset=UTF-8

@geoand geoand self-assigned this Aug 24, 2021
geoand added a commit to geoand/quarkus that referenced this issue Aug 24, 2021
gsmet added a commit that referenced this issue Aug 24, 2021
Add UTF-8 as charset when RESTEasy Reactive returns text/plain
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Aug 24, 2021
@gsmet gsmet modified the milestones: 2.3 - main, 2.2.0.Final Aug 24, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Aug 24, 2021
jsmrcka added a commit to jsmrcka/quarkus-test-suite that referenced this issue Aug 25, 2021
Add `http/jaxrs-reactive`, which is a RESTEasy reactive equivalent of
`http/jaxrs`.
Notable differences and limitations:
- In a multipart POJO, Java type for `image` and `octet-stream` were
  changed from `InputStream` to `File` - `InputStream` is not supported
  by `resteasy-reactive`.
- Test of a functionality provided by the
  `quarkus.resteasy.multipart.input-part.default-charset` property has
  been disabled for now - it is unsupported in `resteasy-reactive`.
  quarkusio/quarkus#19527
- [FIXED] Possible bug in `resteasy-reactive` - endpoints  consuming
  `MULTIPART_FORM_DATA` cause build failure.
  quarkusio/quarkus#19404.
- Tests dealing with `text/plain` request/response have been disabled
  on Windows due to a bug.
  quarkusio/quarkus#19535
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest env/windows Impacts Windows machines kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants