Skip to content

avoid unexpected automapping #895

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

Merged
harawata merged 2 commits into from
Jan 17, 2017
Merged

avoid unexpected automapping #895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50ad409
avoid unexpected automapping when a explicit mapped proerty has the s…
Jan 13, 2017
3568393
unit test for avoiding unexpected automapping issue
Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/apache/ibatis/executor/resultset/DefaultResultSetHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ private List<UnMappedColumnAutoMapping> createAutomaticMappings(ResultSetWrapper
}
}
final String property = metaObject.findProperty(propertyName, configuration.isMapUnderscoreToCamelCase());
if (property != null && metaObject.hasSetter(property)) {
if (property != null && metaObject.hasSetter(property) && !resultMap.getMappedProperties().contains(property)) {
Copy link
Member

@kazuki43zoo kazuki43zoo Jan 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@harawata harawata Jan 17, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@kazuki43zoo kazuki43zoo Jan 17, 2017

Choose a reason for hiding this comment

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

I might not fully understand the expected behavior of autoMappingUnknownColumnBehavior.

See #585.

I will improve a contributing code.

Copy link
Member

@harawata harawata Jan 18, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

@kazuki43zoo kazuki43zoo Jan 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

@harawata harawata Jan 18, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@kazuki43zoo kazuki43zoo Jan 19, 2017

Choose a reason for hiding this comment

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

let's consider when someone desperately wants it ;)

Okay :)

final Class<?> propertyType = metaObject.getSetterType(property);
if (typeHandlerRegistry.hasTypeHandler(propertyType, rsw.getJdbcType(columnName))) {
final TypeHandler<?> typeHandler = rsw.getTypeHandler(propertyType, columnName);
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/org/apache/ibatis/mapping/ResultMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public class ResultMap {
private List<ResultMapping> constructorResultMappings;
private List<ResultMapping> propertyResultMappings;
private Set<String> mappedColumns;
private Set<String> mappedProperties;
private Discriminator discriminator;
private boolean hasNestedResultMaps;
private boolean hasNestedQueries;
Expand Down Expand Up @@ -71,6 +72,7 @@ public ResultMap build() {
throw new IllegalArgumentException("ResultMaps must have an id");
}
resultMap.mappedColumns = new HashSet<String>();
resultMap.mappedProperties = new HashSet<String>();
resultMap.idResultMappings = new ArrayList<ResultMapping>();
resultMap.constructorResultMappings = new ArrayList<ResultMapping>();
resultMap.propertyResultMappings = new ArrayList<ResultMapping>();
Expand All @@ -88,6 +90,10 @@ public ResultMap build() {
}
}
}
final String property = resultMapping.getProperty();
if(property != null) {
resultMap.mappedProperties.add(property);
}
if (resultMapping.getFlags().contains(ResultFlag.CONSTRUCTOR)) {
resultMap.constructorResultMappings.add(resultMapping);
} else {
Expand Down Expand Up @@ -146,6 +152,10 @@ public Set<String> getMappedColumns() {
return mappedColumns;
}

public Set<String> getMappedProperties() {
return mappedProperties;
}

public Discriminator getDiscriminator() {
return discriminator;
}
Expand Down
14 changes: 14 additions & 0 deletions src/test/java/org/apache/ibatis/submitted/automapping/AutomappingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,20 @@ public void shouldGetAUser() {
}
}

@Test
public void shouldGetAUserWhithPhoneNumber() {
sqlSessionFactory.getConfiguration().setAutoMappingBehavior(AutoMappingBehavior.NONE);
SqlSession sqlSession = sqlSessionFactory.openSession();
try {
Mapper mapper = sqlSession.getMapper(Mapper.class);
User user = mapper.getUserWithPhoneNumber(1);
Assert.assertEquals("User1", user.getName());
Assert.assertEquals(new Long(12345678901L), user.getPhone());
} finally {
sqlSession.close();
}
}

@Test
public void shouldNotInheritAutoMappingInherited_InlineNestedResultMap() {
sqlSessionFactory.getConfiguration().setAutoMappingBehavior(AutoMappingBehavior.NONE);
Expand Down
9 changes: 6 additions & 3 deletions src/test/java/org/apache/ibatis/submitted/automapping/CreateDB.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ drop table books if exists;

create table users (
id int,
name varchar(20)
name varchar(20),
phone varchar(20),
phone_number bigint
);

create table books (
Expand All @@ -40,8 +42,9 @@ create table breeder (
name varchar(20)
);

insert into users (id, name) values(1, 'User1');
insert into users (id, name) values(2, 'User2');
-- '+86 12345678901' can't be converted to a number
insert into users (id, name, phone, phone_number) values(1, 'User1', '+86 12345678901', 12345678901);
insert into users (id, name, phone, phone_number) values(2, 'User2', '+86 12345678902', 12345678902);

insert into books (version, name) values(99, 'Learn Java');

Expand Down
2 changes: 2 additions & 0 deletions src/test/java/org/apache/ibatis/submitted/automapping/Mapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public interface Mapper {

User getUser(Integer id);

User getUserWithPhoneNumber(Integer id);

User getUserWithPets_Inline(Integer id);

User getUserWithPets_External(Integer id);
Expand Down
10 changes: 9 additions & 1 deletion src/test/java/org/apache/ibatis/submitted/automapping/Mapper.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,20 @@
<mapper namespace="org.apache.ibatis.submitted.automapping.Mapper">

<select id="getUser" resultMap="result">
select * from users where id = #{id}
select id, name from users where id = #{id}
</select>

<resultMap type="org.apache.ibatis.submitted.automapping.User" id="result" autoMapping="true">
</resultMap>

<select id="getUserWithPhoneNumber" resultMap="resultWithPhoneNumber">
select * from users where id = #{id}
</select>

<resultMap type="org.apache.ibatis.submitted.automapping.User" id="resultWithPhoneNumber" autoMapping="true">
<result property="phone" column="phone_number"/>
</resultMap>

<sql id="selectUserPetBreeder">
select users.id, users.name,
pets.id as petId, pets.name as petName,
Expand Down
10 changes: 10 additions & 0 deletions src/test/java/org/apache/ibatis/submitted/automapping/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public class User {

private Integer id;
private String name;
private Long phone; // phone number of Long type

private List<Pet> pets;

public Integer getId() {
Expand All @@ -39,6 +41,14 @@ public void setName(String name) {
this.name = name;
}

public Long getPhone() {
return phone;
}

public void setPhone(Long phone) {
this.phone = phone;
}

public List<Pet> getPets() {
return pets;
}
Expand Down