Lazily cache request body for Spring requests instead of eagerly caching it

[lee-jinhwan]

:scroll: Description

• Spring provides a ContentCachingRequestWrapper that caches the body.
• ~Support other Content-types, including JSON.~

:bulb: Motivation and Context

• The goal of this PR is to fix Request#getParameter not returning a result if the POST request is application/x-www-form-urlencoded.
• In Request in embed-Tomcat, if the InputStream is read first(body cached), the parameter will not be parsed.

// class org.apache.catalina.connector.Request  
 @Override
    public ServletInputStream getInputStream() throws IOException {

        if (usingReader) {
            throw new IllegalStateException(sm.getString("coyoteRequest.getInputStream.ise"));
        }

        usingInputStream = true;
        if (inputStream == null) {
            inputStream = new CoyoteInputStream(inputBuffer);
        }
        return inputStream;

    }

   ...

 protected void parseParameters() {
   ...
            if (usingInputStream || usingReader) {
                success = true;
                return;
            }
  ...

:green_heart: How did you test it?

:pencil: Checklist

• [X] I reviewed the submitted code.
• [X] I added tests to verify the changes.
• [ ] No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• [ ] I updated the docs if needed.
• [ ] Review from the native team if needed.
• [ ] No breaking change or entry added to the changelog.
• [ ] No breaking change for hybrid SDKs or communicated to hybrid SDKs.

:crystal_ball: Next steps

[lbloder]

Hi @lee-jinhwan, Thanks for providing this PR.

I had a quick look, but unfortunately one our Integration tests is failing due to the changes.

The Test in question is SentrySpringIntegrationTest.attaches request body to SentryEvents, which fails because in this test scenario we are not reading the body of the request, which means the ContentCachingRequestWrapper does not cache the body and thus we only get an empty string in the SentryEvent. This could also happen if the request never gets to a controller because there's an exception in the filter chain, in which case we would not send the request body to Sentry.

Also one of the unit tests fails (SentrySpringFilterTest.caches request depending on the maxRequestBodySize value and request body length) due to the removal of the check for contentType of application/json. We need to discuss internally which mime types we want to support. In this particular test we are not expecting an application/octet-stream to be cached. Also we run into the same problem as in the integration test above, i.e. the content is not actually cached because the stream has never been read.

So the RequestPayloadExtractor might have to somehow check if the body has already been read (i.e. is cached) and the getContentAsString() method can be used or if the stream has to be read.

We will come back to you once we had a quick internal discussion.

@markushi @romtsn @stefanosiano @adinauer wdyt?

[lee-jinhwan]

@lbloder Thank you for your kind review.

I hadn't thought about the case where the body is not read. I fixed the failed test case using inputstream. Also, I reverted the commit related to mime-type. Please consider supporting other mime types.

[lee-jinhwan]

@adinauer Oops, I fixed the test case failure, please run the checks again.

To anyone, what about support for other mimes tpyes? Have you had any discussions about support?

[adinauer]

Not yet, I'm just coming back from vacation.

[adinauer]

@lee-jinhwan we just discussed the PR and would like to retarget it onto the 8.x.x branch so it'll be included in the 8.0 release.

Can you confirm this changes behaviour in the following way (for the changelog): "Lazily cache request body for Spring requests instead of eagerly caching it" In practice the difference should be small since the body should be loaded before it gets to the controller anyways.

Regarding MIME-types, which types would you like to see supported?

[lee-jinhwan]

Changed to the 8.x.x branch target.

JSON is used a lot, but form-urlencoded requests are also used a lot, so I would like to support for “application/x-www-form-urlencoded”.

[adinauer]

Thanks @lee-jinhwan, I've created https://github.com/getsentry/sentry-java/issues/3656 to track the MIME type changes. Should land in 8.0. Can't give an ETA though.

I'll take another look at merging this PR once I'm up to date with PRs and TODOs that should land in 8.x.x so I can come up with a good merge order.

[adinauer]

Hello @lee-jinhwan I just tested the changes and unfortunately the first POST request to the /person endpoint in our Spring Boot 3 sample fails with org.springframework.http.converter.HttpMessageNotReadableException: Required request body is missing: io.sentry.samples.spring.boot.jakarta.Person io.sentry.samples.spring.boot.jakarta.PersonController.create(io.sentry.samples.spring.boot.jakarta.Person)] with the changes made in this PR.

Testing the same thing on the 8.x.x branch succeeds on the first request.

I did not do any investigations as to why this happens.

[adinauer]

@lee-jinhwan it looks like this PR solves a compatiblity issue with Spring Boot 3.4. Thanks again for the PR! We're planning to merge this and release it as part of 8.0.0-rc.3.