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

quarkus-rest-jaxb: Request body is null #43186

Closed
qa-andreas-stangl opened this issue Sep 10, 2024 · 4 comments · Fixed by #43213
Closed

quarkus-rest-jaxb: Request body is null #43186

qa-andreas-stangl opened this issue Sep 10, 2024 · 4 comments · Fixed by #43213
Labels
area/jaxb kind/bug Something isn't working
Milestone

Comments

@qa-andreas-stangl
Copy link
Contributor

qa-andreas-stangl commented Sep 10, 2024

Describe the bug

When making a http request with xml as a request body, The deserialized request body is null.
The project uses quarkus-rest-jaxb for xml support. The endpoint definition is similar to this:

public class MyResource {

    @POST
    @Consumes({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML})
    @Path("my-path")
    public Response myEndpoint(MyRequest request) {
        // request is null
        return ...;
    }
}

Expected behavior

When sending an xml body to the endpoint, the request body is deserialized and passed to the myEndpoint method.

Actual behavior

The request parameter of myEndpoint is null.

How to Reproduce?

Finding exact steps to reproduce the issue might prove difficult since a race condition is likely involved. The issue occurred when testing the application on a testing environment whereas everything worked as expected when running the application locally.

Output of uname -a or ver

No response

Output of java -version

Java 21

Quarkus version or git rev

3.13.1

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

No response

Additional information

I believe this issue is caused by the ServerJaxbMessageBodyReader class:

...

    private Object doReadFrom(Class<Object> type, Type genericType, InputStream entityStream) throws IOException {
        if (isInputStreamEmpty(entityStream)) {
            return null;
        }
    
        return unmarshal(entityStream, type);
    }
    
    private boolean isInputStreamEmpty(InputStream entityStream) throws IOException {
        return StreamUtil.isEmpty(entityStream) || entityStream.available() == 0;
    }
}

entityStream.available() is called to check whether the stream is empty. The documentation for this method states:

Returns an estimate of the number of bytes that can be read (or skipped over) from this input stream without blocking, which may be 0, or 0 when end of stream is detected.

This might lead to a stream wrongfully being classified as empty. No bytes being available to be read without blocking does not necessarily imply that there are no bytes to be read at all.

I have been able to test this theory by attaching a debugger to the application and creating a breakpoint on the return null; line. When I reevaluate isInputStreamEmpty(entityStream) once the
breakpoint has been reached the result is false. Moreover, entityStream.available() is now larger than 0.

This issue can likely be avoided by adjusting the isInputStreamEmpty method and no longer using entityStream.available() to check whether the stream is empty.

A possible workaround for this issue is to add a request filter like this to the project:

import jakarta.ws.rs.container.ContainerRequestContext;
import jakarta.ws.rs.container.ContainerRequestFilter;
import jakarta.ws.rs.ext.Provider;

import java.io.IOException;
import java.io.InputStream;
import java.io.PushbackInputStream;

@Provider
public class EntityPeekingFilter implements ContainerRequestFilter {

    @Override
    public void filter(ContainerRequestContext requestContext) throws IOException {
        if (requestContext.hasEntity()) {
            InputStream entityStream = requestContext.getEntityStream();
            PushbackInputStream pushbackStream = new PushbackInputStream(entityStream);
            int firstByte = pushbackStream.read();
            pushbackStream.unread(firstByte);
            requestContext.setEntityStream(pushbackStream);
        }
    }
}

This request filter reads one byte from the stream and then resets the streams start. This ensures that there is always at least one byte available for reading in a non-blocking manner.

@qa-andreas-stangl qa-andreas-stangl added the kind/bug Something isn't working label Sep 10, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 10, 2024

/cc @gsmet (jaxb)

@gsmet
Copy link
Member

gsmet commented Sep 10, 2024

@geoand this one looks legit and is probably for you!

@geoand
Copy link
Contributor

geoand commented Sep 10, 2024

Thanks for the analysis!

To be honest, I have no idea why entityStream.available() == 0 was added (it wasn't done by me for sure). Moreover, we don't see to use that in any other MessageBodyReader.
I agree we should remove it.

Do you want to open a PR @qa-andreas-stangl ?

@qa-andreas-stangl
Copy link
Contributor Author

I have created a PR such that the stream emptiness check is no longer relies on available(). As it turns out, the additional check entityStream.available() == 0 was not added without reason though. It seems like StreamUtil.isEmpty() is not able to reliably detect empty streams by itsself, for example when the request body is empty. This would then cause an exception during deserialization. My proposed solution is to instead read a byte from the stream and check whether its end has been read. This approach should be able to reliably categorize streams as empty / not empty.

@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/jaxb kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants