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

@Body annotation causes "Request body has already been claimed" error #11266

Closed
pbthorste opened this issue Oct 22, 2024 · 3 comments
Closed
Assignees

Comments

@pbthorste
Copy link

Expected Behavior

Ideally the body parameter should be available for use. This was working in micronaut-starter version 4.5.1.

Actual Behaviour

In some scenarios, using a @Body parameter in a controller causes a "Request body has already been claimed" error.

Steps To Reproduce

Create a controller using micronaut-starter 4.6.3 like this:

import io.micronaut.core.annotation.Nullable;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpResponse;
import io.micronaut.http.annotation.Body;
import io.micronaut.http.annotation.Controller;
import io.micronaut.http.annotation.PathVariable;
import io.micronaut.http.annotation.Post;
import io.micronaut.json.tree.JsonNode;
import reactor.core.publisher.Mono;

@Controller
public class PostController {

    @Post("/{+path}")
    public Mono<HttpResponse<JsonNode>> doPost(@PathVariable String path,
                                               @Nullable @Body Object body,
                                               HttpRequest<?> request) {
        return Mono.just(HttpResponse.ok());
    }
}

Create this unit test:

import io.micronaut.core.type.Argument;
import io.micronaut.http.HttpMethod;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpResponse;
import io.micronaut.http.HttpStatus;
import io.micronaut.http.client.annotation.Client;
import io.micronaut.reactor.http.client.ReactorHttpClient;
import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
import jakarta.inject.Inject;
import org.junit.jupiter.api.Test;

import java.util.Map;

import static org.junit.jupiter.api.Assertions.*;

@MicronautTest
class PostControllerTest {

    @Inject
    @Client("/")
    ReactorHttpClient client;

    @Test
    void ok_request_with_body() {
        var post = HttpRequest.POST( "/do-post", Map.of("key1", "val1"));

        HttpResponse<Map<String, Object>> result = client.toBlocking().exchange(post, Argument.mapOf(String.class, Object.class));
        assertEquals(HttpStatus.OK, result.getStatus());
    }
}

Running this test will give this error:

12:42:04.980 [default-nioEventLoopGroup-1-3] ERROR i.m.http.server.RouteExecutor - Unexpected error occurred: Request body has already been claimed: Two conflicting sites are trying to access the request body. If this is intentional, the first user must ByteBody#split the body. To find out where the body was claimed, 

Environment Information

JDK version 21.
OS version: Mac os

Example Application

No response

Version

4.6.3

@pbthorste
Copy link
Author

There is a workaround to get the body, not sure if this always works but here it is:

@Controller
public class PostController {

    @Post("/{+path}")
    public Mono<HttpResponse<Object>> doPost(@PathVariable String path,
                                               HttpRequest<?> request) {
        try(CloseableByteBody copy = ((NettyHttpRequest<?>) request).byteBody().split(ByteBody.SplitBackpressureMode.SLOWEST).allowDiscard()) {
            return Mono.from(copy.toByteArrayPublisher())
                    .onErrorComplete(ByteBody.BodyDiscardedException.class)
                    .flatMap(bodyBytes -> {
                      String body = new String(bodyBytes);
                      return Mono.just(HttpResponse.ok(body));
                    });
        }
    }
}

@graemerocher
Copy link
Contributor

correct workaround is to use Map<String, Object> as the body for the moment

@pbthorste
Copy link
Author

Thanks @graemerocher . I changed the controller method and it works. It now looks like this:

    @Post("/{+path}")
    public Mono<HttpResponse<Object>> doPost(@PathVariable String path,
                                             @Nullable @Body Map<String, Object> body,
                                             HttpRequest<?> request) {
        return Mono.just(HttpResponse.ok());
    }

yawkat added a commit that referenced this issue Oct 22, 2024
There's two bugs here.

Let's consider the following cases:

1. `application/x-www-form-urlencoded`, `@Body Object` param (tested by the old FormDataDiskSpec)
2. `application/json`, `@Body Object` param (prev. no test, tested by a new TCK test)
3. `application/x-weird`, `@Body Object` param (prev. no test, tested by a new netty-specific test)
4. `application/x-weird`, `@Body MyRecord` param (prev. no test, tested by a new netty-specific test)

Prior to 4.6, case 2 has a registered "normal" reader (the JSON reader) that can handle this case fine, the other three cases do not have readers and rely on fallback conversion logic. Forms have some special handling here.

In 4.6, the way raw type readers (e.g. that for String) were resolved changed. This means that for `Object` params, all raw type readers were now eligible, since they can read a subtype of Object (e.g. String). For cases 1, 3, and 4, this is a breaking change, since the reading is now done by a (random) raw type reader, instead of falling back to conversion logic.

This change broke the FormDataDiskSpec (case 1). To fix this, 1d09d95 introduced a check that would blanket-ignore the reader for all Object-typed params, falling back to the conversion logic. This fixed cases 1 and 3, but inadvertently broke case 2, since the JSON reader was also ignored.

Case 2 is apparently common enough that there have been multiple reports about it. It also exposed the second bug, a double-claim of the body, which is a fairly simple fix. But it shows that the fallback conversion logic had no test coverage, since this bug should trigger reliably. That is what `BodyConversionSpec` is there to improve.

My proposed fix is to make the raw message readers not apply to Object parameters, and to remove the band-aid fix from 1d09d95 that broke case 2.

Fixes #11266
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants