Add test coverage for improvement multipart encoded mode in rest-client

[jcarranzan]

Summary

Adding TD for the next backport:
Improve the multipart encoded mode handling in the rest client

This PR adds test coverage for the improved multipart encoder mode handling in the REST client.
The tests cover the encoder modes (HTML5, RFC1738, RFC3986), verifying the correct encoding of multipart requests and successful parsing by a Vert.x server. Data integrity checks, including file content verification via size and headers, are performed.

Note: Direct verification of the specific encoder mode used during the transmission is not directly feasible, but
the tests ensure that the resulting content is correctly parsed and interpreted by the server, providing confidence that the appropriate encoder mode was applied.

Please select the relevant options.

• Bug fix (non-breaking change which fixes an issue)
• Dependency update
• Refactoring
• Backport
• New scenario (non-breaking change which adds functionality)
• This change requires a documentation update
• This change requires execution against OCP (use run tests phrase in comment)

Checklist:

• Methods and classes used in PR scenarios are meaningful
• Commits are well encapsulated and follow the best practices

[github-actions]

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

[github-actions]

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

[github-actions]

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'

[rsvoboda]

Hi @jcarranzan.

I did the first quick check of this PR and there are some things I would like to have:

• PR description doesn't add too much details about the test coverage, it points to the PR where I can get to the discussion, but that's 2 hops, short summary would make the life better for the reviewer
• Short scenario summary should be present in HttpSimpleEncodeModeIT as JavaDoc, people from the team will be looking into this class sooner or later and having some context is better than needing to go through the commits history and reading the code
• Having stuff in io.quarkus.ts.http.restclient.reactive is not intuitive as you are mixing new classes with the original ones, put new classes into io.quarkus.ts.http.restclient.reactive.multipart. Test can stay in the same package as it is now.
• What's the motivation to use Vert.x server as the second server and not 2 Quarkus applications, just curious, not asking to rework stuff
• I didn't notice any check to verify that the different encoderMode was used (not sure there is an easy way to do, but this is a thing that could be mentioned in PR description that it was considered), the test is checking that the correct content was received, but not how it was encoded ... in theory the multipart could be encoded using the same strategy all the time

[jcarranzan]

Hi @jcarranzan.

I did the first quick check of this PR and there are some things I would like to have:

* PR description doesn't add too much details about the test coverage, it points to the PR where I can get to the discussion, but that's 2 hops, short summary would make the life better for the reviewer

* Short scenario summary should be present in HttpSimpleEncodeModeIT as JavaDoc, people from the team will be looking into this class sooner or later and having some context is better than needing to go through the commits history and reading the code

* Having stuff in `io.quarkus.ts.http.restclient.reactive` is not intuitive as you are mixing new classes with the original ones, put new classes into `io.quarkus.ts.http.restclient.reactive.multipart`. Test can stay in the same package as it is now.

* What's the motivation to use Vert.x server as the second server and not 2 Quarkus applications, just curious, not asking to rework stuff

* I didn't notice any check to verify that the different `encoderMode` was used (not sure there is an easy way to do, but this is a thing that could be mentioned in PR description that it was considered), the test is checking that the correct content was received, but not how it was encoded ... in theory the multipart could be encoded using the same strategy all the time

I agree, I will improve it with your suggestions, thanks @rsvoboda .

[michalvavrik]

I didn't notice any check to verify that the different encoderMode was used ......

All that is true, but it's not possible to check this. Netty doesn't provide any API access to raw message that could give us a clue. Decoder result is always true. IMO:

• only way to check specific mode is to have access to lower lever, so that we could do check similar to this: https://github.com/netty/netty/blob/151dfa083d28e995a18f7d2c73d4a7d3b7ab73b2/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoderTest.java#L264
  • it will require different stack or some method I missed
• only way to check that different mode is used when different configuration property is set would be to reproduce this MicroProfile Rest Client - Multiple multipart files quarkusio/quarkus#39751 (reply in thread), however it works for multiple files regardless of modes, so there must be something else, like "you cannot reproduce it without undertow" or something; this can be done

[michalvavrik]

hmm, I wonder if you could test this with Wiremock? Let's try that.

[jcarranzan]

I've improved the next steps:

• PR description
• JavaDoc in the test class
• Rename of the test class

Answering some questions here:

1. What's the motivation to use Vert.x server as the second server and not 2 Quarkus applications...

While initially, it seemed possible to verify the encoding mode directly using the Vert.x server as a lightweight option, further investigation didn't reveal a straightforward approach. I didn' consider about 2 Quarkus application, but yes could be other option.

2. @rsvoboda about the last point "I didn't notice any check to verify that the different encoderMode was used (not sure there is an easy way to do, but this is a thing that could be mentioned in PR description that it was considered), the test is checking that the correct content was received, but not how it was encoded ... in theory the multipart could be encoded using the same strategy all the time"

It's true, directly inspecting the encoding mode in transit isn't straightforward, and I though checking the structure of the received multipart content was enough. I also tried the reproduced described here quarkusio/quarkus#39751 (reply in thread) but I noticed some things as server-undertow, etc that was out of scope of this testing.

PW: I am trying to figure out other ways to testing the encode mode as the Wiremock to see if it's possible at this moment...

[rsvoboda]

What about proposed Wiremock approach? That could be a follow-up, not necessary to be in this PR.

[rsvoboda]

I think this is good enough.

I have 2 suggestions for the future improvement if you feel like it:

• content of the uploaded files could be checked ... extend the DTO and store there buffer.toString()
• add negative scenario - e.g. with incorrect multipart-post-encoder-mode

[jcarranzan]

Ok now I've added those last 2 suggestions (check for the content of the uploaded files and the negative scenario). I will push them in some minutes.

[github-actions]

Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version'