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.

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

Choose a constructor for auto-mapping suing TypeHandler #1277

Merged
merged 3 commits into from May 15, 2018

Conversation

Projects
None yet
3 participants
@h3adache
Copy link
Member

h3adache commented May 7, 2018

I enhanced the behavior of constructor automapping by using the type registry.
I also added @jeffgbutler's suggestion to always use solo constructors.
This handles about 90% of the use cases I've seen so far lol.
Incorrect types the error msg is good enough which was my only concern.
I removed WrapperSubject and it's test since that's best left for a different discussion (which I started earlier).

Related Issues

@h3adache h3adache requested review from harawata , jeffgbutler and kazuki43zoo May 7, 2018

kazuki43zoo and others added some commits Apr 2, 2018

Enhance fix for #1240 using @jeffgbutler always choose if single cons…
…tructor suggestion

Also use type registry instead of primitivetypes checking as it's more
extensive and allows for better constructor mapping behavior.

@h3adache h3adache force-pushed the pr-1240 branch from 7221b7a to b20cf31 May 7, 2018

@h3adache

This comment has been minimized.

Copy link
Member Author

h3adache commented May 7, 2018

Sorry fixed the PR. I must have made a mistake when pulling from @kazuki43zoo's branch.

if (parameterType.isPrimitive() && !primitiveTypes.getWrapper(parameterType).getName().equals(classNames.get(i))) {
return false;
} else if (!parameterType.isPrimitive() && !parameterType.getName().equals(classNames.get(i))) {
if (!typeHandlerRegistry.hasTypeHandler(parameterTypes[i], jdbcTypes.get(i))) {
return false;
}
}
return true;
}

This comment has been minimized.

@harawata

harawata May 9, 2018

Member

Isn't it better to use the old allowedConstructor() here?
As you found out, TypeHandlerRegistry#hasTypeHandler() returns true for most basic Java types regardless of the specified JDBC type.

This comment has been minimized.

@h3adache

h3adache May 9, 2018

Author Member

Yea that means that multiple constructors it will choose the first that passes.
I think that's ok as it works for the majority of the cases.
As you mentioned in the previous code, choosing correct types isn't so simple.
TypeHandlers is a very powerful feature and leveraging it is definitely a positive.
It's also more consistent with how automapping are currently done.
As for the old allowedConstructor, it was more restrictive as initially my aim was just to slightly improve the existing autoconstructor mapping.

This comment has been minimized.

@harawata

harawata May 10, 2018

Member

I see. If it's intentional, I'm OK with it. :)

@h3adache

This comment has been minimized.

Copy link
Member Author

h3adache commented May 9, 2018

should also fix #1200

@h3adache

This comment has been minimized.

Copy link
Member Author

h3adache commented May 15, 2018

Thanks for the look @harawata!

@h3adache h3adache merged commit fd7fc5f into master May 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@h3adache h3adache deleted the pr-1240 branch May 15, 2018

@kazuki43zoo kazuki43zoo added this to the 3.5.0 milestone May 16, 2018

@kazuki43zoo kazuki43zoo changed the title Fix for #1240 Choose a constructor for auto-mapping suing TypeHandler May 16, 2018

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented May 16, 2018

@h3adache
I've changed the PR title. If my modification is wrong, plz modify it :D
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment