Skip to content

Commit

Permalink
fixes #721 Allow specifying constructor arg by its parameter name.
Browse files Browse the repository at this point in the history
  • Loading branch information
harawata committed Jan 24, 2017
1 parent 39573e3 commit 9095710
Show file tree
Hide file tree
Showing 15 changed files with 668 additions and 5 deletions.
4 changes: 3 additions & 1 deletion src/main/java/org/apache/ibatis/annotations/Arg.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Copyright 2009-2016 the original author or authors.
* Copyright 2009-2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -44,4 +44,6 @@
String select() default "";

String resultMap() default "";

String name() default "";
}
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,7 @@ private void applyConstructorArgs(Arg[] args, Class<?> resultType, List<ResultMa
(arg.typeHandler() == UnknownTypeHandler.class ? null : arg.typeHandler());
ResultMapping resultMapping = assistant.buildResultMapping(
resultType,
null,
nullOrEmpty(arg.name()),
nullOrEmpty(arg.column()),
arg.javaType() == void.class ? null : arg.javaType(),
arg.jdbcType() == JdbcType.UNDEFINED ? null : arg.jdbcType(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* Copyright 2009-2016 the original author or authors.
* Copyright 2009-2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -358,7 +358,12 @@ private boolean databaseIdMatchesCurrent(String id, String databaseId, String re
}

private ResultMapping buildResultMappingFromContext(XNode context, Class<?> resultType, List<ResultFlag> flags) throws Exception {
String property = context.getStringAttribute("property");
String property;
if (flags.contains(ResultFlag.CONSTRUCTOR)) {
property = context.getStringAttribute("name");
} else {
property = context.getStringAttribute("property");
}
String column = context.getStringAttribute("column");
String javaType = context.getStringAttribute("javaType");
String jdbcType = context.getStringAttribute("jdbcType");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="UTF-8" ?>
<!--
Copyright 2009-2016 the original author or authors.
Copyright 2009-2017 the original author or authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -89,6 +89,7 @@ jdbcType CDATA #IMPLIED
typeHandler CDATA #IMPLIED
select CDATA #IMPLIED
resultMap CDATA #IMPLIED
name CDATA #IMPLIED
>

<!ELEMENT arg EMPTY>
Expand All @@ -99,6 +100,7 @@ jdbcType CDATA #IMPLIED
typeHandler CDATA #IMPLIED
select CDATA #IMPLIED
resultMap CDATA #IMPLIED
name CDATA #IMPLIED
>

<!ELEMENT collection (constructor?,id*,result*,association*,collection*, discriminator?)>
Expand Down
91 changes: 91 additions & 0 deletions src/main/java/org/apache/ibatis/mapping/ResultMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,30 @@
*/
package org.apache.ibatis.mapping;

import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;

import org.apache.ibatis.annotations.Param;
import org.apache.ibatis.builder.BuilderException;
import org.apache.ibatis.logging.Log;
import org.apache.ibatis.logging.LogFactory;
import org.apache.ibatis.reflection.Jdk;
import org.apache.ibatis.reflection.ParamNameUtil;
import org.apache.ibatis.session.Configuration;

/**
* @author Clinton Begin
*/
public class ResultMap {
private Configuration configuration;

private String id;
private Class<?> type;
private List<ResultMapping> resultMappings;
Expand All @@ -45,13 +56,16 @@ private ResultMap() {
}

public static class Builder {
private static final Log log = LogFactory.getLog(Builder.class);

private ResultMap resultMap = new ResultMap();

public Builder(Configuration configuration, String id, Class<?> type, List<ResultMapping> resultMappings) {
this(configuration, id, type, resultMappings, null);
}

public Builder(Configuration configuration, String id, Class<?> type, List<ResultMapping> resultMappings, Boolean autoMapping) {
resultMap.configuration = configuration;
resultMap.id = id;
resultMap.type = type;
resultMap.resultMappings = resultMappings;
Expand All @@ -76,6 +90,7 @@ public ResultMap build() {
resultMap.idResultMappings = new ArrayList<ResultMapping>();
resultMap.constructorResultMappings = new ArrayList<ResultMapping>();
resultMap.propertyResultMappings = new ArrayList<ResultMapping>();
final List<String> constructorArgNames = new ArrayList<String>();
for (ResultMapping resultMapping : resultMap.resultMappings) {
resultMap.hasNestedQueries = resultMap.hasNestedQueries || resultMapping.getNestedQueryId() != null;
resultMap.hasNestedResultMaps = resultMap.hasNestedResultMaps || (resultMapping.getNestedResultMapId() != null && resultMapping.getResultSet() == null);
Expand All @@ -96,6 +111,9 @@ public ResultMap build() {
}
if (resultMapping.getFlags().contains(ResultFlag.CONSTRUCTOR)) {
resultMap.constructorResultMappings.add(resultMapping);
if (resultMapping.getProperty() != null) {
constructorArgNames.add(resultMapping.getProperty());
}
} else {
resultMap.propertyResultMappings.add(resultMapping);
}
Expand All @@ -106,6 +124,13 @@ public ResultMap build() {
if (resultMap.idResultMappings.isEmpty()) {
resultMap.idResultMappings.addAll(resultMap.resultMappings);
}
if (!constructorArgNames.isEmpty()) {
if (!sortConstructorResultMapping(constructorArgNames)) {
throw new BuilderException("Failed to find a constructor in '"
+ resultMap.getType().getName() + "' by arg names " + constructorArgNames
+ ". There might be more info in debug log.");
}
}
// lock down collections
resultMap.resultMappings = Collections.unmodifiableList(resultMap.resultMappings);
resultMap.idResultMappings = Collections.unmodifiableList(resultMap.idResultMappings);
Expand All @@ -114,6 +139,72 @@ public ResultMap build() {
resultMap.mappedColumns = Collections.unmodifiableSet(resultMap.mappedColumns);
return resultMap;
}

private boolean sortConstructorResultMapping(final List<String> constructorArgNames) {
Constructor<?>[] constructors = resultMap.type.getDeclaredConstructors();
// Search constructors by arg names and types.
for (Constructor<?> constructor : constructors) {
Class<?>[] paramTypes = constructor.getParameterTypes();
if (constructorArgNames.size() == paramTypes.length) {
final List<String> paramNames = getArgNames(constructor);
if (constructorArgNames.containsAll(paramNames)) {
if (!argTypesMatch(constructorArgNames, paramTypes, paramNames)) {
continue;
}
// Found a matching constructor.
Collections.sort(resultMap.constructorResultMappings, new Comparator<ResultMapping>() {
@Override
public int compare(ResultMapping o1, ResultMapping o2) {
int paramIdx1 = paramNames.indexOf(o1.getProperty());
int paramIdx2 = paramNames.indexOf(o2.getProperty());
return paramIdx1 - paramIdx2;
}
});
return true;
}
}
}
return false;
}

private boolean argTypesMatch(final List<String> constructorArgNames,
Class<?>[] paramTypes, List<String> paramNames) {
for (int i = 0; i < constructorArgNames.size(); i++) {
Class<?> actualType = paramTypes[paramNames.indexOf(constructorArgNames.get(i))];
Class<?> specifiedType = resultMap.constructorResultMappings.get(i).getJavaType();
if (!actualType.equals(specifiedType)) {
if (log.isDebugEnabled()) {
log.debug("Found a constructor with arg names " + constructorArgNames
+ ", but the type of '" + constructorArgNames.get(i)
+ "' did not match. Specified: [" + specifiedType.getName() + "] Declared: ["
+ actualType.getName() + "]");
}
return false;
}
}
return true;
}

private List<String> getArgNames(Constructor<?> constructor) {
if (resultMap.configuration.isUseActualParamName() && Jdk.parameterExists) {
return ParamNameUtil.getParamNames(constructor);
} else {
List<String> paramNames = new ArrayList<String>();
final Annotation[][] paramAnnotations = constructor.getParameterAnnotations();
int paramCount = paramAnnotations.length;
for (int paramIndex = 0; paramIndex < paramCount; paramIndex++) {
String name = null;
for (Annotation annotation : paramAnnotations[paramIndex]) {
if (annotation instanceof Param) {
name = ((Param) annotation).value();
break;
}
}
paramNames.add(name != null ? name : "arg" + paramIndex);

This comment has been minimized.

Copy link
@kazuki43zoo

kazuki43zoo Jan 28, 2017

Member

Hi @harawata, I have a trivial question !
A fallback name is starting with arg rather than param. Is it intentional ?

Note : A fallback name for method argument is starting with param.

This comment has been minimized.

Copy link
@harawata

harawata Jan 28, 2017

Author Member

Thanks for checking!
It could be anything. I chose 'arg' only because it is also used by Java 8 compiler when '-parameters' is not specified when there is no parameter names stored.

I didn't want users to use these indexed names to specify the constructor args because it is as fragile as the old way, in my opinion.
That is why I didn't mention it in the doc.

If you think these indexed names are useful and 'param' is better, I do not mind changing it at all :)

This comment has been minimized.

Copy link
@kazuki43zoo

kazuki43zoo Jan 28, 2017

Member

I agree with your opinion !
I think no problem in current implementation :)

Thanks for answer !

This comment has been minimized.

Copy link
@harawata

harawata Jan 28, 2017

Author Member

Cool! Thank you for your time!

}
return paramNames;
}
}
}

public String getId() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
--
-- Copyright 2009-2017 the original author or authors.
--
-- Licensed under the Apache License, Version 2.0 (the "License");
-- you may not use this file except in compliance with the License.
-- You may obtain a copy of the License at
--
-- http://www.apache.org/licenses/LICENSE-2.0
--
-- Unless required by applicable law or agreed to in writing, software
-- distributed under the License is distributed on an "AS IS" BASIS,
-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-- See the License for the specific language governing permissions and
-- limitations under the License.
--

drop table users if exists;

create table users (
id int,
name varchar(20),
team int
);

insert into users (id, name, team) values
(1, 'User1', 99);
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/**
* Copyright 2009-2017 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.ibatis.submitted.named_constructor_args;

import static org.hamcrest.CoreMatchers.*;

import java.io.Reader;
import java.sql.Connection;

import org.apache.ibatis.annotations.Arg;
import org.apache.ibatis.annotations.ConstructorArgs;
import org.apache.ibatis.annotations.Select;
import org.apache.ibatis.builder.BuilderException;
import org.apache.ibatis.io.Resources;
import org.apache.ibatis.jdbc.ScriptRunner;
import org.apache.ibatis.session.Configuration;
import org.apache.ibatis.session.SqlSession;
import org.apache.ibatis.session.SqlSessionFactory;
import org.apache.ibatis.session.SqlSessionFactoryBuilder;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

public class InvalidNamedConstructorArgsTest {
@Rule
public ExpectedException ex = ExpectedException.none();

private static SqlSessionFactory sqlSessionFactory;

@BeforeClass
public static void setUp() throws Exception {
// create an SqlSessionFactory
Reader reader = Resources.getResourceAsReader(
"org/apache/ibatis/submitted/named_constructor_args/mybatis-config.xml");
sqlSessionFactory = new SqlSessionFactoryBuilder().build(reader);
reader.close();

// populate in-memory database
SqlSession session = sqlSessionFactory.openSession();
Connection conn = session.getConnection();
reader = Resources
.getResourceAsReader("org/apache/ibatis/submitted/named_constructor_args/CreateDB.sql");
ScriptRunner runner = new ScriptRunner(conn);
runner.setLogWriter(null);
runner.runScript(reader);
reader.close();
session.close();
}

interface NoMatchingConstructorMapper {
@ConstructorArgs({
@Arg(column = "id", name = "noSuchConstructorArg"),
})
@Select("select * from users ")
User select();
}

@Test
public void noMatchingConstructorArgName() {
ex.expect(BuilderException.class);
ex.expectMessage(startsWith("Failed to find a constructor in "
+ "'org.apache.ibatis.submitted.named_constructor_args.User' by arg names [noSuchConstructorArg]."));

Configuration configuration = sqlSessionFactory.getConfiguration();
configuration.addMapper(NoMatchingConstructorMapper.class);
}

interface ConstructorWithWrongJavaType {
// There is a constructor with arg name 'id', but
// its type is different from the specified javaType.
@ConstructorArgs({
@Arg(column = "id", name = "id", javaType = Integer.class),
})
@Select("select * from users ")
User select();
}

@Test
public void wrongJavaType() {
ex.expect(BuilderException.class);
ex.expectMessage(startsWith("Failed to find a constructor in "
+ "'org.apache.ibatis.submitted.named_constructor_args.User' by arg names [id]."));

Configuration configuration = sqlSessionFactory.getConfiguration();
configuration.addMapper(ConstructorWithWrongJavaType.class);
}

interface ConstructorMissingRequiresJavaType {
// There is a constructor with arg name 'id', but its type
// is different from the type of a property with the same name.
// javaType is required in this case.
// Debug log shows the detail of the matching error.
@ConstructorArgs({
@Arg(column = "id", name = "id"),
})
@Select("select * from users ")
User select();
}

@Test
public void missingRequiredJavaType() {
ex.expect(BuilderException.class);
ex.expectMessage(startsWith("Failed to find a constructor in "
+ "'org.apache.ibatis.submitted.named_constructor_args.User' by arg names [id]."));

Configuration configuration = sqlSessionFactory.getConfiguration();
configuration.addMapper(ConstructorMissingRequiresJavaType.class);
}
}
Loading

0 comments on commit 9095710

Please sign in to comment.