Skip to content

Fix UrlPathHelper#shouldRemoveSemicolonContent() #27303

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
Merged
merged 1 commit into from
Aug 22, 2021
Merged

Fix UrlPathHelper#shouldRemoveSemicolonContent() #27303

merged 1 commit into from
Aug 22, 2021

Conversation

evpaassen
Copy link
Contributor

@evpaassen evpaassen commented Aug 20, 2021 •
edited
Loading

Fixes #27256

This PR fixes a bug that renders read-only UrlPathHelpers unusable, because shouldRemoveSemicolonContent() throws an exception. For example, when using UrlPathHelper#rawPathInstance, which is read-only.

The checkReadOnly() method should only be called from methods that modify properties to prevent modification of read-only instances.

@evpaassen evpaassen mentioned this pull request Aug 20, 2021
Closed
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 20, 2021
@sbrannen
Copy link
Member

Hi @evpaassen,

Can you please introduce a unit test that fails prior to the change and passes after the change?

@sbrannen sbrannen self-assigned this Aug 20, 2021
@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 20, 2021
@sbrannen sbrannen added this to the 5.3.10 milestone Aug 20, 2021
@evpaassen
Copy link
Contributor Author

Hi @sbrannen,

I added a test now. I'm not sure it really has added value, because nothing regarding the readOnly property in this class was tested before, and the new test is effectively testing a getter method. But on the other hand, it will prevent a regression, I guess.

Please let me know if you're happy with it, or that I should make any changes.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 20, 2021
@evpaassen
Fix UrlPathHelper#shouldRemoveSemicolonContent()
51a1731 
The checkReadOnly() method should only be called from
methods that modify properties to prevent modification
of read-only instances.

Fixes spring-projects#27256
@sbrannen
Copy link
Member

I added a test now. I'm not sure it really has added value, because nothing regarding the readOnly property in this class was tested before, and the new test is effectively testing a getter method. But on the other hand, it will prevent a regression, I guess.

Yes, my intent was to make sure the problem is fixed and does not return (i.e., to serve as a regression test).

Please let me know if you're happy with it, or that I should make any changes.

That looks good now.

Thanks

@sbrannen sbrannen merged commit 462e19d into spring-projects:main Aug 22, 2021
@evpaassen evpaassen deleted the fix-27256 branch August 22, 2021 12:20
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.2.x labels Aug 22, 2021
@spring-projects-issues spring-projects-issues mentioned this pull request Aug 22, 2021
Closed
@sbrannen sbrannen removed the status: feedback-provided Feedback has been provided label Aug 22, 2021
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Aug 22, 2021
@sbrannen
Polish contribution
99970a5 
See spring-projectsgh-27303
@evpaassen
Copy link
Contributor Author

You're welcome. And thanks for reviewing and merging!

sbrannen pushed a commit to sbrannen/spring-framework that referenced this pull request Aug 22, 2021
@evpaassen @sbrannen
Fix UrlPathHelper#shouldRemoveSemicolonContent() (spring-projects#27303)
10a90d3 
The checkReadOnly() method should only be called from
methods that modify properties to prevent modification
of read-only instances.

Fixes spring-projects#27256
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Aug 22, 2021
@sbrannen
Polish contribution
97a89d6 
See spring-projectsgh-27303
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Aug 22, 2021
@sbrannen
Fix Checkstyle violation
b28f403 
See spring-projectsgh-27303
@sbrannen
Copy link
Member

Note that this was also backported to 5.2.17 in 10a90d3.

@sync-by-unito sync-by-unito bot mentioned this pull request Sep 15, 2021
Closed
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@evpaassen @lxbzmy
Fix UrlPathHelper#shouldRemoveSemicolonContent() (spring-projects#27303)
de4fdb6 
The checkReadOnly() method should only be called from
methods that modify properties to prevent modification
of read-only instances.

Fixes spring-projects#27256
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@sbrannen @lxbzmy
Polish contribution
3f0caf2 
See spring-projectsgh-27303
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@sbrannen @lxbzmy
Fix Checkstyle violation
0003b1b 
See spring-projectsgh-27303
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: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Milestone
5.3.10
Development

Successfully merging this pull request may close these issues.

UrlPathHelper: checkReadOnly() called in read method shouldRemoveSemicolonContent()
3 participants
@evpaassen @sbrannen @spring-projects-issues