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.

Sign up for GitHub

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

Jump to bottom

Fix condition for "Too many elements" in MimeTypeUtils.sortBySpecificity() #31769

Closed
wants to merge 1 commit into from
Closed

Fix condition for "Too many elements" in MimeTypeUtils.sortBySpecificity() #31769

wants to merge 1 commit into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Dec 6, 2023

This PR fixes condition for "Too many elements" in MimeTypeUtils.sortBySpecificity() that seems to have been changed accidentally in 05c3ffb to align with its Javadoc again.

See gh-31254

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 6, 2023
@sbrannen sbrannen self-assigned this Dec 6, 2023
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Dec 6, 2023
@sbrannen sbrannen changed the title Fix condition for "Too many elements" in MimeTypeUtils.sortBySpecificity() Fix condition for "Too many elements" in MimeTypeUtils.sortBySpecificity() Dec 6, 2023
@sbrannen sbrannen added type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 6, 2023
@sbrannen sbrannen added this to the 6.1.2 milestone Dec 6, 2023
@sbrannen
Copy link
Member

sbrannen commented Dec 6, 2023

Good catch!

Although this is very minor, it is technically a regression.

So I've labeled it as such, and we'll back port it to 6.0.x

@sbrannen sbrannen added for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x in: core Issues in core modules (aop, beans, core, context, expression) and removed in: web Issues in web modules (web, webmvc, webflux, websocket) labels Dec 6, 2023
@sbrannen sbrannen mentioned this pull request Dec 6, 2023
Closed
@sbrannen sbrannen added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x labels Dec 6, 2023
@sbrannen sbrannen closed this in 7b95bd7 Dec 6, 2023
sbrannen pushed a commit that referenced this pull request Dec 6, 2023
@izeye @sbrannen
Fix condition for "Too many elements" in MimeTypeUtils.sortBySpecific…
fa95f12 
…ity()

See gh-31254
See gh-31769
Closes gh-31773

(cherry picked from commit 7b95bd7)
@izeye izeye deleted the gh-31254 branch December 6, 2023 13:36
@jandroalvarez
Copy link

jandroalvarez commented Mar 18, 2024 •
edited

@sbrannen in #31254 it was notified MimeTypeUtils.sortBySpecificity was throwing an IllegalArgumentException when the number of MimeTypes in the Accept header was greater than 50.

To fix it, the proposed change was to modify HeaderContentNegotiationStrategy to catch IllegalArgumentExceptiion instead of InvalidMediaTypeException (InvalidMediaTypeException is a sub-type of IllegalArgumentException).

Instead, the assert that was throwing the IllegalArgumentException was replaced by an if condition that throws InvalidMimeTypeException which is a sub-type of IllegalArgumentException.

Therefore, the same runtime problem is being reproduced. Only changed IllegalArgumentException with InvalidMimeTypeException.

This problem was reported in this issue and it was just changed the if statement replacing the >= with >.

Why haven't you changed the catch clause as originally requested in #31254? The same runtime crash is still happening.

@sbrannen

This comment was marked as outdated.

Sign in to view
@sbrannen

This comment was marked as outdated.

Sign in to view
@sbrannen
Copy link
Member

sbrannen commented Mar 18, 2024 •
edited

@jandroalvarez, to ensure the issue you've reported does not get lost, I am going to go ahead and create a new issue based on your input now.

So, there's no need for you to create a new issue.

@sbrannen sbrannen mentioned this pull request Mar 18, 2024
Closed
sbrannen added a commit that referenced this pull request Mar 19, 2024
@sbrannen
Wrap InvalidMimeTypeException in HttpMediaTypeNotAcceptableException
ef02f0b 
The fix for #31254 resulted in an InvalidMimeTypeException being thrown
by MimeTypeUtils.sortBySpecificity() instead of an
IllegalArgumentException. However, InvalidMimeTypeException extends
IllegalArgumentException. Consequently, the change from
IllegalArgumentException to InvalidMimeTypeException did not result in
the desired effect in HeaderContentNegotiationStrategy.

HeaderContentNegotiationStrategy.resolveMediaTypes() still allows the
InvalidMimeTypeException to propagate as-is without wrapping it in an
HttpMediaTypeNotAcceptableException.

To address this issue, this commit catches InvalidMediaTypeException
and InvalidMimeTypeException in HeaderContentNegotiationStrategy and
wraps the exception in an HttpMediaTypeNotAcceptableException.

See gh-31254
See gh-31769
Closes gh-32483
sbrannen added a commit that referenced this pull request Mar 19, 2024
@sbrannen
Wrap InvalidMimeTypeException in HttpMediaTypeNotAcceptableException
da799bc 
The fix for #31254 resulted in an InvalidMimeTypeException being thrown
by MimeTypeUtils.sortBySpecificity() instead of an
IllegalArgumentException. However, InvalidMimeTypeException extends
IllegalArgumentException. Consequently, the change from
IllegalArgumentException to InvalidMimeTypeException did not result in
the desired effect in HeaderContentNegotiationStrategy.

HeaderContentNegotiationStrategy.resolveMediaTypes() still allows the
InvalidMimeTypeException to propagate as-is without wrapping it in an
HttpMediaTypeNotAcceptableException.

To address this issue, this commit catches InvalidMediaTypeException
and InvalidMimeTypeException in HeaderContentNegotiationStrategy and
wraps the exception in an HttpMediaTypeNotAcceptableException.

See gh-31254
See gh-31769
Closes gh-32483

(cherry picked from commit ef02f0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sbrannen sbrannen

Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Projects
None yet
Milestone
6.1.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@izeye @sbrannen @jandroalvarez @spring-projects-issues