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

avoid unexpected automapping #895

Merged
merged 2 commits into from Jan 17, 2017

Conversation

Projects
None yet
4 participants
@dulong

dulong commented Jan 13, 2017

avoid unexpected automapping when a explicit mapped proerty has the same name as another column

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Jan 13, 2017

Member

Could you provide a test case?

Member

harawata commented Jan 13, 2017

Could you provide a test case?

@dulong

This comment has been minimized.

Show comment
Hide comment
@dulong

dulong Jan 17, 2017

@harawata Hi, I added a test case to the existed auto mapping test suite

dulong commented Jan 17, 2017

@harawata Hi, I added a test case to the existed auto mapping test suite

@harawata harawata self-assigned this Jan 17, 2017

@harawata harawata added this to the 3.4.3 milestone Jan 17, 2017

@harawata harawata merged commit 978b922 into mybatis:master Jan 17, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Jan 17, 2017

Member

Thank you, @dulong ! =D

Member

harawata commented Jan 17, 2017

Thank you, @dulong ! =D

@kazuki43zoo

This comment has been minimized.

Show comment
Hide comment
@kazuki43zoo

kazuki43zoo Jan 17, 2017

Member

@harawata , Is this enhancement or bug ?

Member

kazuki43zoo commented Jan 17, 2017

@harawata , Is this enhancement or bug ?

@harawata

This comment has been minimized.

Show comment
Hide comment
@harawata

harawata Jan 17, 2017

Member

A bug, I would say.

Member

harawata commented Jan 17, 2017

A bug, I would say.

@kazuki43zoo kazuki43zoo added bug and removed enhancement labels Jan 17, 2017

@kazuki43zoo

This comment has been minimized.

Show comment
Hide comment
@kazuki43zoo

kazuki43zoo Jan 17, 2017

Member

Do not need a warning log in this case ?

Member

kazuki43zoo commented Jan 17, 2017

Do not need a warning log in this case ?

@@ -477,7 +477,7 @@ private Object getPropertyMappingValue(ResultSet rs, MetaObject metaResultObject
}
}
final String property = metaObject.findProperty(propertyName, configuration.isMapUnderscoreToCamelCase());
if (property != null && metaObject.hasSetter(property)) {
if (property != null && metaObject.hasSetter(property) && !resultMap.getMappedProperties().contains(property)) {

This comment has been minimized.

@kazuki43zoo

kazuki43zoo Jan 17, 2017

Member

I think it should not use && condition in this case.

Because...
If resultMap.getMappedProperties().contains(property) is true, execute following code.

configuration.getAutoMappingUnknownColumnBehavior()
        .doAction(mappedStatement, columnName, (property != null) ? property : propertyName, null);

In this case, i think it is already mapped property rather than not unknown column.

@kazuki43zoo

kazuki43zoo Jan 17, 2017

Member

I think it should not use && condition in this case.

Because...
If resultMap.getMappedProperties().contains(property) is true, execute following code.

configuration.getAutoMappingUnknownColumnBehavior()
        .doAction(mappedStatement, columnName, (property != null) ? property : propertyName, null);

In this case, i think it is already mapped property rather than not unknown column.

This comment has been minimized.

@harawata

harawata Jan 17, 2017

Member

I might not fully understand the expected behavior of autoMappingUnknownColumnBehavior.
Feel free to commit the necessary changes :)

@harawata

harawata Jan 17, 2017

Member

I might not fully understand the expected behavior of autoMappingUnknownColumnBehavior.
Feel free to commit the necessary changes :)

This comment has been minimized.

@kazuki43zoo

kazuki43zoo Jan 17, 2017

Member

I might not fully understand the expected behavior of autoMappingUnknownColumnBehavior.

See #585.

I will improve a contributing code.

@kazuki43zoo

kazuki43zoo Jan 17, 2017

Member

I might not fully understand the expected behavior of autoMappingUnknownColumnBehavior.

See #585.

I will improve a contributing code.

This comment has been minimized.

@harawata

harawata Jan 18, 2017

Member

OK..I have to admit that I am a little bit confused 😆
Please let me clarify.

When running the contributed test case (AutomappingTest.shouldGetAUserWhithPhoneNumber()), four columns (id, name, phone and phone_number) are returned from the query and only three of them are mapped.

  • 'phone_number' is explicitly mapped
  • 'id' and 'name' are auto-mapped

I thought that the unmapped column 'phone' is an unknown column, but it actually isn't, is that correct?

@harawata

harawata Jan 18, 2017

Member

OK..I have to admit that I am a little bit confused 😆
Please let me clarify.

When running the contributed test case (AutomappingTest.shouldGetAUserWhithPhoneNumber()), four columns (id, name, phone and phone_number) are returned from the query and only three of them are mapped.

  • 'phone_number' is explicitly mapped
  • 'id' and 'name' are auto-mapped

I thought that the unmapped column 'phone' is an unknown column, but it actually isn't, is that correct?

This comment has been minimized.

@kazuki43zoo

kazuki43zoo Jan 18, 2017

Member

Hi @harawata

In My opinion, the phone is valid column name as auto-mapping (if does not use explicit mapping...), it skip because it has been mapped by explicit mapping already. Hence i think phone is not unknown column.
I think we should consider to apply a mechanism that notify duplicate mapping for same property (like same as autoMappingUnknownColumnBehavior).

for example:

  • NONE: skip auto-mapping
  • WARNING: skip auto-mapping and output warning log that notify duplicate mapping for same property
  • FAILING : throw exception that that notify duplicate mapping for same property

What do you think ?

@kazuki43zoo

kazuki43zoo Jan 18, 2017

Member

Hi @harawata

In My opinion, the phone is valid column name as auto-mapping (if does not use explicit mapping...), it skip because it has been mapped by explicit mapping already. Hence i think phone is not unknown column.
I think we should consider to apply a mechanism that notify duplicate mapping for same property (like same as autoMappingUnknownColumnBehavior).

for example:

  • NONE: skip auto-mapping
  • WARNING: skip auto-mapping and output warning log that notify duplicate mapping for same property
  • FAILING : throw exception that that notify duplicate mapping for same property

What do you think ?

This comment has been minimized.

@harawata

harawata Jan 18, 2017

Member

Hence i think phone is not unknown column.

Okay, I have no strong opinion about this.

I think we should consider to apply a mechanism that notify duplicate mapping for same property (like same as autoMappingUnknownColumnBehavior).

The duplicate mapping situation like this would be rare.
And this unknown column detection can be inaccurate [1] anyway, so let's consider when someone desperately wants it ;)

[1] There will be false-positives with a nested result map.

@harawata

harawata Jan 18, 2017

Member

Hence i think phone is not unknown column.

Okay, I have no strong opinion about this.

I think we should consider to apply a mechanism that notify duplicate mapping for same property (like same as autoMappingUnknownColumnBehavior).

The duplicate mapping situation like this would be rare.
And this unknown column detection can be inaccurate [1] anyway, so let's consider when someone desperately wants it ;)

[1] There will be false-positives with a nested result map.

This comment has been minimized.

@kazuki43zoo

kazuki43zoo Jan 19, 2017

Member

let's consider when someone desperately wants it ;)

Okay :)

@kazuki43zoo

kazuki43zoo Jan 19, 2017

Member

let's consider when someone desperately wants it ;)

Okay :)

kazuki43zoo added a commit to kazuki43zoo/mybatis-3 that referenced this pull request Jan 17, 2017

@kazuki43zoo

This comment has been minimized.

Show comment
Hide comment
@kazuki43zoo

kazuki43zoo Jan 17, 2017

Member

Hi @harawata and @dulong,
I've improved contributing code via a5895f7. If my change has problem, please post comment.

Thanks.

Member

kazuki43zoo commented Jan 17, 2017

Hi @harawata and @dulong,
I've improved contributing code via a5895f7. If my change has problem, please post comment.

Thanks.

@dulong

This comment has been minimized.

Show comment
Hide comment
@dulong

dulong Jan 18, 2017

Hi @kazuki43zoo , thanks for the improvement, i appreciated that 👍

dulong commented Jan 18, 2017

Hi @kazuki43zoo , thanks for the improvement, i appreciated that 👍

@jarvan4dev

This comment has been minimized.

Show comment
Hide comment
@jarvan4dev

jarvan4dev Mar 22, 2017

@dulong Hello, Could you give me a hand? #585

jarvan4dev commented Mar 22, 2017

@dulong Hello, Could you give me a hand? #585

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