Add quarkus.rest-client.multipart.max-chunk-size property

[ejba]

Closes #35915

[ejba]

WDYT @geoand?

[geoand]

Looks good on a first pass.

Have you tested it?

[ejba]

@geoand I added some unit tests. I'm trying to include some integration tests but I'm lacking creativity at the moment. Any suggestion?

[geoand]

I would like to see something like:

public class ChunkedSizeTest {

    @RegisterExtension
    static final QuarkusUnitTest TEST = new QuarkusUnitTest()
            .withApplicationRoot((jar) -> jar.addClasses(TestClient.class, TestResource.class))
            .withConfigurationResource("chunked-size-test-application.properties");

    @Test
    public void test() throws Exception {
        // ensure that the response is what you expect
    }

    @Path("test")
    public interface TestClient {
        @POST
        @Produces(MediaType.TEXT_PLAIN)
        @Consumes(MediaType.TEXT_PLAIN)
        String test(byte[] input);
    }

    @Path("test")
    public static class TestResource {

        @POST
        @Produces(MediaType.TEXT_PLAIN)
        @Consumes(MediaType.TEXT_PLAIN)
        public String test(@Context HttpHeaders headers, byte[] input) {
            return input.length + "/" + headers.getLength() + "/" + headers.getHeaderString("transfer-encoding");
        }
    }
}

[geoand]

BTW, has this been tested against the server API mentioned in #35915?

[ejba]

BTW, has this been tested against the server API mentioned in #35915?

Not yet.

[ejba]

@geoand what is the expected behaviour when the multipart request content fits into a single chunk?

quarkus/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/multipart/PausableHttpPostRequestEncoder.java

Line 845 in f974941

if (realSize > QuarkusHttpPostBodyUtil.chunkSize || isMultipart) {

The http client chunks unconditionally the multipart requests.

Although there are servers which does not support payloads encoded into chunks, as trello API for example.

[geoand]

Hm... We took that from Netty and this issue seems to be exactly the same.

If I were you, I'd remove that check for multipart in a separate commit and run the tests to see what happens.

[ejba]

Wondering if the property name is explicit enough for developers to learn on their own that it is a specific configuration for multi-part requests. It might be wiser to add the intermediate level ("quarkus.rest-client.multipart.max-chunk-size") to make the configuration explict. WDTY?

[geoand]

It might be wiser to add the intermediate level ("quarkus.rest-client.multipart.max-chunk-size") to make the configuration explict. WDTY?

+1

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[geoand]

Can you explain these changes please?

[ejba]

Without these changes, shouldSendFromMultiEmittingOutsideEventLoop and shouldUploadSlowlyProducedData test cases will fail (as you can see in the previous CI report).

The fetched HTTP chunk can be a null value and the request's content has to be marked as chunked because of that (based on my observation). Otherwise, an NPE will be thrown because the source buffer (i.e. the chunk) is a null value.

The reason for the buffer to be a null value on these test cases is unfortunately to be determined.

[geoand]

Okay, I think we can live with it as long as CI passes and as long as #35915 is fixed

[ejba]

We could ask to the netty team what they think about these changes.

[geoand]

Let's not burden them unless we really need to.

[geoand]

Thanks a lot for this @ejba!

Have you tried this against the use case mentioned in #35915?

[ejba]

Have you tried this against the use case mentioned in #35915?

No, I haven't yet. Honestly I think the added integration tests are similar to the use case mentioned in #35915 which gave my confidence to consider the PR as "ready". However, I agree it should be tested against to trello server.

[geoand]

The test looks good I agree, but we really need to test this against the real problematic server otherwise we can't close the issue 😉

[ejba]

Set the max-chunk-size property to 200_000.

[geoand]

That's awesome, thanks!