Fix controller @Body Object parameters
#11267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
graemerocher
approved these changes
Oct 22, 2024
|
@yawkat can you change to graalvm 1.2.4 in your pr so the the build passes and we can merge it? |
|
dstepanov
reviewed
Oct 24, 2024
| @@ -38,6 +38,6 @@ public interface TypedMessageBodyReader<T> extends MessageBodyReader<T> { | |||
|
|
|||
| @Override | |||
| default boolean isReadable(Argument<T> type, MediaType mediaType) { | |||
| return type.isAssignableFrom(getType()); | |||
| return type.isAssignableFrom(getType()) && !type.getType().equals(Object.class); | |||
There was a problem hiding this comment.
I don't think this a correct fix, typed reader should only add a type and nothing else
There was a problem hiding this comment.
I'm open to suggestions. The new test cases cover the issue pretty well.
|
Let's keep it this way. We shouldn't allow Object for body in the future |
dstepanov
approved these changes
Oct 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's two bugs here.
Let's consider the following cases:
application/x-www-form-urlencoded,@Body Objectparam (tested by the old FormDataDiskSpec)application/json,@Body Objectparam (prev. no test, tested by a new TCK test)application/x-weird,@Body Objectparam (prev. no test, tested by a new netty-specific test)application/x-weird,@Body MyRecordparam (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
Objectparams, 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
BodyConversionSpecis 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