Fix "array index out of bounds" problem reported by LGTM.com

[jbduncan]

This PR is a follow-on to #23461.

Specifically the problem that this PR fixes is https://lgtm.com/projects/g/spring-projects/spring-framework/snapshot/fb09647cbd34d6a540dc6e9b7e86aa7bcbe8e223/files/spring-web/src/main/java/org/springframework/http/ContentDisposition.java#xcfd962975fb7b533:1.

Feedback and other thoughts are welcome!

[pivotal-issuemaster]

@jbduncan Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@jbduncan Thank you for signing the Contributor License Agreement!

[rstoyanchev]

Could you re-work the tests to avoid the use reflection?

[mentallurg]

@rstoyanchev: The only reason for reflection is to test private method. Then one should implement a test for public methods. I find it better. But the tests already contained 2 tests for exactly this same private method and they of course use reflection. Be consistent. Either accept new reflection based tests or rework other already existing reflection based tests.

[mentallurg]

I find the reflection based tests OK in this case. If one prefers not to use reflection in the tests, then it should be handled in a separate issue, and that would be a huge change, because there are already many reflection based tests in Spring.

[jbduncan]

Hi @rstoyanchev @mentallurg, sorry for the delay in responding! I was on holiday and then catching up with work and IRL things.

I wasn't sure about introducing a reflection-based test either, but I used the same reasoning that @mentallurg did and I decided to use reflection for consistency with the other tests.

@rstoyanchev Would you prefer that I switch all the reflection-based tests in ContentDispositionTests to use the public API instead?

I'm not sure if either of you are Spring maintainers, but is there anything else you'd like me to do before this PR is merged in? :)

[sbrannen]

I'm not sure if either of you are Spring maintainers

@jbduncan, you can look at comments from people to see if there is a Member button, like the one you see for Rossen here: #23485 (comment)

The People page can also be of assistance for the entire Spring portfolio: https://github.com/orgs/spring-projects/people

[sbrannen]

I wasn't sure about introducing a reflection-based test either, but I used the same reasoning that @mentallurg did and I decided to use reflection for consistency with the other tests.

That's valid reasoning.

However, I don't see any reason that encodeHeaderFieldParam() and decodeHeaderFieldParam() should be private. Changing their visibility to package-private would remove the need for reflection in the tests since ContentDispositionTests resides in the same package as ContentDisposition, and making that change shouldn't hurt anything with regard to encapsulation.

@rstoyanchev, WDYT?

[jbduncan]

Hey @sbrannen, fancy meeting you here. :)

@jbduncan, you can look at comments from people to see if there is a Member button, like the one you see for Rossen here: #23485 (comment)

The People page can also be of assistance for the entire Spring portfolio: https://github.com/orgs/spring-projects/people

Oh cool, thank you for reminding me about that button and the existence of GitHub's "People" tab. Cheers! 👍

[rstoyanchev]

This PR has been merged. I removed the use of reflection by doing actual parsing and/or formatting with an encoded filename attribute. There were even some existing tests like that so I don't know why we had reflection there in the first place.

The refactoring helped to uncover a small issue with tolerating "filename*" without a charset (i.e. US_ASCII) which the decode method was dealing with but the duplicate code in parse wasn't.

[jbduncan]

Thanks @rstoyanchev! 👍