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

Exclude Part from nested constructor binding in WebFlux #31778

Closed
wants to merge 2 commits into from
Closed

Exclude Part from nested constructor binding in WebFlux #31778

wants to merge 2 commits into from

Conversation

HeartPattern
Copy link
Contributor

Related to: #31669 and 9ade52d

WebFlux has same issue with nullable multipart file handling. MultipartFile and Part should be excluded from nested constructor binding in WebExchangeDataBinder.

@pivotal-cla
Copy link

@HeartPattern 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-cla
Copy link

@HeartPattern Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 7, 2023
@sbrannen sbrannen requested a review from rstoyanchev December 8, 2023 13:07
sbrannen
sbrannen requested changes Dec 8, 2023
View reviewed changes
Copy link
Member

@sbrannen sbrannen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please introduce a test that fails before the change and passes after the change?

@sbrannen sbrannen added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) labels Dec 8, 2023
@sbrannen sbrannen changed the title Exclude Part and MultipartFile from nested constructor binding on WebFlux Exclude Part and MultipartFile from nested constructor binding on WebFlux Dec 8, 2023
@sbrannen sbrannen mentioned this pull request Dec 8, 2023
Closed
@sbrannen sbrannen changed the title Exclude Part and MultipartFile from nested constructor binding on WebFlux Exclude Part and MultipartFile from nested constructor binding in WebFlux Dec 8, 2023
@HeartPattern HeartPattern requested a review from sbrannen December 9, 2023 13:44
@HeartPattern
Copy link
Contributor Author

HeartPattern commented Dec 9, 2023 •
edited
Loading

However, it seems like Part and FilePart is not the only problem. Any other nullable types is not bind properly through constructor after ea398d7. For example, in kotlin

data class MyBean(
  val anotherBean: AnotherBean?,
)

Maybe checking param.isOptional() == false before shouldConstructArgument() is proper fix for this issue.

@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 Dec 9, 2023
@sbrannen
Copy link
Member

sbrannen commented Dec 9, 2023

However, it seems like Part and FilePart is not the only problem. Any other nullable types is not bind properly through constructor after ea398d7. For example, in kotlin

@HeartPattern, can you please create a separate issue to discuss that?

@HeartPattern
Copy link
Contributor Author

Open another PR addressing that problem.
#31800

@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 status: feedback-provided Feedback has been provided labels Dec 11, 2023
@sbrannen sbrannen added this to the 6.1.2 milestone Dec 11, 2023
rstoyanchev
rstoyanchev reviewed Dec 12, 2023
View reviewed changes
spring-web/src/main/java/org/springframework/web/bind/support/WebExchangeDataBinder.java
Class<?> type = param.nestedIfOptional().getNestedParameterType();
return (super.shouldConstructArgument(param) &&
!MultipartFile.class.isAssignableFrom(type) && !Part.class.isAssignableFrom(type));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a necessary change indeed for Part but MultipartFile is not used in WebFlux, so we can leave that out.

@rstoyanchev rstoyanchev self-assigned this Dec 12, 2023
@rstoyanchev
Copy link
Contributor

I'm processing this locally as we are close to the 6.1.2 release. No need send further updates.

rstoyanchev pushed a commit that referenced this pull request Dec 12, 2023
@HeartPattern @rstoyanchev
Exclude Part from nested constructor binding in WebFlux
4d838c1 
See gh-31778
@rstoyanchev rstoyanchev changed the title Exclude Part and MultipartFile from nested constructor binding in WebFlux Exclude Part from nested constructor binding in WebFlux Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rstoyanchev rstoyanchev rstoyanchev left review comments

@sbrannen sbrannen sbrannen approved these changes

Assignees

@rstoyanchev rstoyanchev

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) 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.
5 participants
@HeartPattern @pivotal-cla @sbrannen @rstoyanchev @spring-projects-issues