Skip to content

Honor generic type information in BeanUtils.copyProperties() #24281

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
Closed
wants to merge 5 commits into from
Closed

Honor generic type information in BeanUtils.copyProperties() #24281

wants to merge 5 commits into from

Conversation

kpkunal1406
Copy link

Fixes #24187 Honor generic type information when copying properties with BeanUtils

Handled standard supported Type Casting of properties. Any custom Object level property casting is restricted.

Kunal Patel added 3 commits January 2, 2020 19:22
SPR-24187 Enhanced datatype conversion possibilities while copying pr…
f3e1844 
…operties with BeanUtils
Enhanced type conversion while copying properties
818f636 
SPR-24187 Enhanced datatype conversion possibilities while copying properties with BeanUtils
Enhanced Datatype conversion while copying properties
d309ef0 
SPR-24187 Enhanced datatype conversion possibilities while copying properties with BeanUtils
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 2, 2020
@kpkunal1406 kpkunal1406 mentioned this pull request Jan 2, 2020
Closed
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Jan 9, 2020
@sbrannen
Copy link
Member

sbrannen commented Jan 10, 2020 •
edited
Loading

Thanks for the PR.

In #24187, my understanding is that we would want to honor generics when matching properties to copy.

In other words, I don't think we actually want to convert properties, since that might lead to unexpected results -- for example, loss of information when converting from Float to Integer in the tests in this PR.

As a side note...

Note: Due to its reliance on PropertyEditors, SimpleTypeConverter is not thread-safe. Use a separate instance for each thread.

@kpkunal1406
Copy link
Author

kpkunal1406 commented Jan 19, 2020 •
edited
Loading

@sbrannen ,

Thanks for your feedback.
If we do not convert properties then target property's datatype will remain as source (As per current implementation no need to change anything). for e.g. List<Integer> is copied into List<String>. Here generic datatype is List. But when we actually fetch it target property it will be List<Integer>.

And for conversion from Float to Integer in test case is as sample example. User must be knowing while copying properties from source to target (what he/she is doing). This can be taken care by the developer.

Without any conversion how can we achieve this?

or else you mean to say allow to copy only if both (source & target) property's datatypes are matching otherwise skip (no java.lang.ClassCastException)?

@sbrannen
Copy link
Member

sbrannen commented Feb 4, 2020

or else you mean to say allow to copy only if both (source & target) property's datatypes are matching otherwise skip (no java.lang.ClassCastException)?

Yes, I was saying that I think we would only want to copy properties if they are compatible without conversion.

@jhoeller, what are your thoughts on the matter?

@kpkunal1406 kpkunal1406 reopened this Mar 13, 2020
@kpkunal1406
Copy link
Author

@sbrannen ,

I have pushed new implementation to copy properties without conversion. And it copies only if source and target property's types are matching.

@sbrannen sbrannen self-assigned this Apr 8, 2020
sbrannen
sbrannen reviewed Apr 13, 2020
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.

Thanks for the modifying the original PR.

I have reviewed the changes and added comments.

Please note, however, that I do not expect you to implement the changes, since I have already done so here (https://github.com/sbrannen/spring-framework/commits/issues/gh-24187-bean-utils-copy-properties-with-generic-types).

It turned out easier for me that way.

Once we branch for 5.3, I will merge my feature branch and keep your commit to give you credit for the work you've done to get the ball rolling with this feature enhancement.

@sbrannen sbrannen changed the title Handled Type Conversion in Properties Copying with BeanUtils Honor generic type information in BeanUtils.copyProperties() Apr 13, 2020
@sbrannen sbrannen removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 13, 2020
@sbrannen sbrannen added this to the 5.3 M1 milestone Apr 13, 2020
@sbrannen sbrannen marked this pull request as draft April 13, 2020 13:34
@kpkunal1406
Copy link
Author

@sbrannen
I appreciate your feedback and suggestions. It is very helpful.

You have already done changes in https://github.com/sbrannen/spring-framework/commits/issues/gh-24187-bean-utils-copy-properties-with-generic-types, I am resolving review comments.
However I have pushed changes into this request also.

Thanks

sbrannen pushed a commit that referenced this pull request Apr 28, 2020
@sbrannen
Honor generic type information in BeanUtils.copyProperties()
89ee0b0 
Prior to this commit, BeanUtils.copyProperties() ignored generic type
information when comparing candidate source and target property types.

This commit reworks the implementation of BeanUtils.copyProperties() so
that generic type information is taken into account when copying
properties.

See gh-24281
@sbrannen
Copy link
Member

This has been merged into master in 89ee0b0 and revised in e74f868.

@sbrannen sbrannen closed this Apr 28, 2020
kenny5he pushed a commit to kenny5he/spring-framework that referenced this pull request Jun 21, 2020
@kenny5he
Honor generic type information in BeanUtils.copyProperties()
73b4d73 
Prior to this commit, BeanUtils.copyProperties() ignored generic type
information when comparing candidate source and target property types.

This commit reworks the implementation of BeanUtils.copyProperties() so
that generic type information is taken into account when copying
properties.

See spring-projectsgh-24281
@sbrannen sbrannen mentioned this pull request Feb 11, 2021
Closed
sbrannen added a commit that referenced this pull request Feb 11, 2021
@sbrannen
Ignore generics when Proxy is supplied to BeanUtils.copyProperties()
667256a 
gh-24281 introduced support to honor generic type information in
BeanUtils.copyProperties(), but that introduced a regression.
Specifically, if the supplied source or target object lacked generic
type information for the return type of the read-method or the
parameter type of the write-method for a given property, respectively,
the two properties would be considered a mismatch and ignored. This can
occur if the source or target object is a java.lang.reflect.Proxy since
the dynamically generated class for the proxy loses the generic type
information from interfaces that the proxy implements.

This commit fixes this regression by ignoring generic type information
if either the source or target property is lacking generic type
information.

Closes gh-26531
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@sbrannen @lxbzmy
Ignore generics when Proxy is supplied to BeanUtils.copyProperties()
79454e9 
spring-projectsgh-24281 introduced support to honor generic type information in
BeanUtils.copyProperties(), but that introduced a regression.
Specifically, if the supplied source or target object lacked generic
type information for the return type of the read-method or the
parameter type of the write-method for a given property, respectively,
the two properties would be considered a mismatch and ignored. This can
occur if the source or target object is a java.lang.reflect.Proxy since
the dynamically generated class for the proxy loses the generic type
information from interfaces that the proxy implements.

This commit fixes this regression by ignoring generic type information
if either the source or target property is lacking generic type
information.

Closes spring-projectsgh-26531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbrannen sbrannen sbrannen left review comments

Assignees

@sbrannen sbrannen

Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Milestone
5.3 M1
Development

Successfully merging this pull request may close these issues.

Honor generic type information when copying properties with BeanUtils
3 participants
@kpkunal1406 @sbrannen @spring-projects-issues