Skip to content

When javaType has not (or partially) been specified, determine the best matching constructor #3378

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
400cdd9
#2618 When `javaType` has not (or partially) been specified, try to d…
epochcoder Jan 3, 2025
0ebdcd2
Running impsort
epochcoder Jan 3, 2025
aa1f246
Merge branch 'master' into feat/2618-auto-determine-type-when-missing
epochcoder Jan 4, 2025
541fc61
Added coverage for failing cases
epochcoder Jan 4, 2025
b5a8c62
Merge branch 'master' into feat/2618-auto-determine-type-when-missing
epochcoder Jan 5, 2025
6b12edc
feat: Centralise constructor resolver
epochcoder Jan 6, 2025
01cbc91
Merge branch 'master' into feat/2618-auto-determine-type-when-missing
epochcoder Jan 6, 2025
7b3e6d2
fix: isEmpty instead of length() == 0 in MapperAnnotationBuilder
epochcoder Jan 6, 2025
009e7dd
Merge branch 'master' into feat/2618-auto-determine-type-when-missing
epochcoder Jan 7, 2025
98665bc
Merge branch 'master' into feat/2618-auto-determine-type-when-missing
epochcoder Jan 12, 2025
3fb8ede
Add missing field in copy constructor
epochcoder Jan 12, 2025
90cf124
Remove unused Log reference
harawata Jan 8, 2025
d17b3fb
Deprecate public methods (just for formality)
harawata Jan 11, 2025
2c20337
Remove `name` attribute from where it is irrelevant
harawata Jan 12, 2025
45716cf
Merge branch 'master' into feat/2618-auto-determine-type-when-missing
epochcoder Jan 15, 2025
be848e4
fix(#2932): remove ability to partially specify property name for con…
epochcoder Jan 15, 2025
f801726
Minor adjustments for cases where constructor mapping does not use ac…
harawata Jan 24, 2025
add7b26
Improved error message that might save an exhausted developer dealing…
harawata Jan 25, 2025
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2009-2024 the original author or authors.
* Copyright 2009-2025 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 @@ -466,5 +466,4 @@ private Class<?> resolveParameterJavaType(Class<?> resultType, String property,
}
return javaType;
}

}

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2009-2024 the original author or authors.
* Copyright 2009-2025 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 @@ -63,6 +63,7 @@
import org.apache.ibatis.builder.CacheRefResolver;
import org.apache.ibatis.builder.IncompleteElementException;
import org.apache.ibatis.builder.MapperBuilderAssistant;
import org.apache.ibatis.builder.ResultMappingConstructorResolver;
import org.apache.ibatis.builder.xml.XMLMapperBuilder;
import org.apache.ibatis.cursor.Cursor;
import org.apache.ibatis.executor.keygen.Jdbc3KeyGenerator;
Expand Down Expand Up @@ -237,7 +238,7 @@ private String generateResultMapName(Method method) {
private void applyResultMap(String resultMapId, Class<?> returnType, Arg[] args, Result[] results,
TypeDiscriminator discriminator) {
List<ResultMapping> resultMappings = new ArrayList<>();
applyConstructorArgs(args, returnType, resultMappings);
applyConstructorArgs(args, returnType, resultMappings, resultMapId);
applyResults(results, returnType, resultMappings);
Discriminator disc = applyDiscriminator(resultMapId, returnType, discriminator);
// TODO add AutoMappingBehaviour
Expand All @@ -251,7 +252,7 @@ private void createDiscriminatorResultMaps(String resultMapId, Class<?> resultTy
String caseResultMapId = resultMapId + "-" + c.value();
List<ResultMapping> resultMappings = new ArrayList<>();
// issue #136
applyConstructorArgs(c.constructArgs(), resultType, resultMappings);
applyConstructorArgs(c.constructArgs(), resultType, resultMappings, resultMapId);
applyResults(c.results(), resultType, resultMappings);
// TODO add AutoMappingBehaviour
assistant.addResultMap(caseResultMapId, c.type(), resultMapId, null, resultMappings, null);
Expand Down Expand Up @@ -461,15 +462,15 @@ private void applyResults(Result[] results, Class<?> resultType, List<ResultMapp

private String findColumnPrefix(Result result) {
String columnPrefix = result.one().columnPrefix();
if (columnPrefix.length() < 1) {
if (columnPrefix.isEmpty()) {
columnPrefix = result.many().columnPrefix();
}
return columnPrefix;
}

private String nestedResultMapId(Result result) {
String resultMapId = result.one().resultMap();
if (resultMapId.length() < 1) {
if (resultMapId.isEmpty()) {
resultMapId = result.many().resultMap();
}
if (!resultMapId.contains(".")) {
Expand All @@ -479,15 +480,15 @@ private String nestedResultMapId(Result result) {
}

private boolean hasNestedResultMap(Result result) {
if (result.one().resultMap().length() > 0 && result.many().resultMap().length() > 0) {
if (!result.one().resultMap().isEmpty() && !result.many().resultMap().isEmpty()) {
throw new BuilderException("Cannot use both @One and @Many annotations in the same @Result");
}
return result.one().resultMap().length() > 0 || result.many().resultMap().length() > 0;
return !result.one().resultMap().isEmpty() || !result.many().resultMap().isEmpty();
}

private String nestedSelectId(Result result) {
String nestedSelect = result.one().select();
if (nestedSelect.length() < 1) {
if (nestedSelect.isEmpty()) {
nestedSelect = result.many().select();
}
if (!nestedSelect.contains(".")) {
Expand All @@ -498,22 +499,24 @@ private String nestedSelectId(Result result) {

private boolean isLazy(Result result) {
boolean isLazy = configuration.isLazyLoadingEnabled();
if (result.one().select().length() > 0 && FetchType.DEFAULT != result.one().fetchType()) {
if (!result.one().select().isEmpty() && FetchType.DEFAULT != result.one().fetchType()) {
isLazy = result.one().fetchType() == FetchType.LAZY;
} else if (result.many().select().length() > 0 && FetchType.DEFAULT != result.many().fetchType()) {
} else if (!result.many().select().isEmpty() && FetchType.DEFAULT != result.many().fetchType()) {
isLazy = result.many().fetchType() == FetchType.LAZY;
}
return isLazy;
}

private boolean hasNestedSelect(Result result) {
if (result.one().select().length() > 0 && result.many().select().length() > 0) {
if (!result.one().select().isEmpty() && !result.many().select().isEmpty()) {
throw new BuilderException("Cannot use both @One and @Many annotations in the same @Result");
}
return result.one().select().length() > 0 || result.many().select().length() > 0;
return !result.one().select().isEmpty() || !result.many().select().isEmpty();
}

private void applyConstructorArgs(Arg[] args, Class<?> resultType, List<ResultMapping> resultMappings) {
private void applyConstructorArgs(Arg[] args, Class<?> resultType, List<ResultMapping> resultMappings,
String resultMapId) {
final List<ResultMapping> mappings = new ArrayList<>();
for (Arg arg : args) {
List<ResultFlag> flags = new ArrayList<>();
flags.add(ResultFlag.CONSTRUCTOR);
Expand All @@ -527,12 +530,16 @@ private void applyConstructorArgs(Arg[] args, Class<?> resultType, List<ResultMa
nullOrEmpty(arg.column()), arg.javaType() == void.class ? null : arg.javaType(),
arg.jdbcType() == JdbcType.UNDEFINED ? null : arg.jdbcType(), nullOrEmpty(arg.select()),
nullOrEmpty(arg.resultMap()), null, nullOrEmpty(arg.columnPrefix()), typeHandler, flags, null, null, false);
resultMappings.add(resultMapping);
mappings.add(resultMapping);
}

final ResultMappingConstructorResolver resolver = new ResultMappingConstructorResolver(configuration, mappings,
resultType, resultMapId);
resultMappings.addAll(resolver.resolveWithConstructor());
}

private String nullOrEmpty(String value) {
return value == null || value.trim().length() == 0 ? null : value;
return value == null || value.trim().isEmpty() ? null : value;
}

private KeyGenerator handleSelectKeyAnnotation(SelectKey selectKeyAnnotation, String baseStatementId,
Expand Down
26 changes: 19 additions & 7 deletions src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2009-2024 the original author or authors.
* Copyright 2009-2025 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 @@ -31,6 +31,7 @@
import org.apache.ibatis.builder.IncompleteElementException;
import org.apache.ibatis.builder.MapperBuilderAssistant;
import org.apache.ibatis.builder.ResultMapResolver;
import org.apache.ibatis.builder.ResultMappingConstructorResolver;
import org.apache.ibatis.cache.Cache;
import org.apache.ibatis.executor.ErrorContext;
import org.apache.ibatis.io.Resources;
Expand Down Expand Up @@ -223,12 +224,17 @@ private ResultMap resultMapElement(XNode resultMapNode, List<ResultMapping> addi
if (typeClass == null) {
typeClass = inheritEnclosingType(resultMapNode, enclosingType);
}

String id = resultMapNode.getStringAttribute("id", resultMapNode::getValueBasedIdentifier);
String extend = resultMapNode.getStringAttribute("extends");
Boolean autoMapping = resultMapNode.getBooleanAttribute("autoMapping");

Discriminator discriminator = null;
List<ResultMapping> resultMappings = new ArrayList<>(additionalResultMappings);
List<XNode> resultChildren = resultMapNode.getChildren();
for (XNode resultChild : resultChildren) {
if ("constructor".equals(resultChild.getName())) {
processConstructorElement(resultChild, typeClass, resultMappings);
processConstructorElement(resultChild, typeClass, resultMappings, id);
} else if ("discriminator".equals(resultChild.getName())) {
discriminator = processDiscriminatorElement(resultChild, typeClass, resultMappings);
} else {
Expand All @@ -239,9 +245,7 @@ private ResultMap resultMapElement(XNode resultMapNode, List<ResultMapping> addi
resultMappings.add(buildResultMappingFromContext(resultChild, typeClass, flags));
}
}
String id = resultMapNode.getStringAttribute("id", resultMapNode::getValueBasedIdentifier);
String extend = resultMapNode.getStringAttribute("extends");
Boolean autoMapping = resultMapNode.getBooleanAttribute("autoMapping");

ResultMapResolver resultMapResolver = new ResultMapResolver(builderAssistant, id, typeClass, extend, discriminator,
resultMappings, autoMapping);
try {
Expand All @@ -265,16 +269,24 @@ protected Class<?> inheritEnclosingType(XNode resultMapNode, Class<?> enclosingT
return null;
}

private void processConstructorElement(XNode resultChild, Class<?> resultType, List<ResultMapping> resultMappings) {
private void processConstructorElement(XNode resultChild, Class<?> resultType, List<ResultMapping> resultMappings,
String id) {
List<XNode> argChildren = resultChild.getChildren();

final List<ResultMapping> mappings = new ArrayList<>();
for (XNode argChild : argChildren) {
List<ResultFlag> flags = new ArrayList<>();
flags.add(ResultFlag.CONSTRUCTOR);
if ("idArg".equals(argChild.getName())) {
flags.add(ResultFlag.ID);
}
resultMappings.add(buildResultMappingFromContext(argChild, resultType, flags));

mappings.add(buildResultMappingFromContext(argChild, resultType, flags));
}

final ResultMappingConstructorResolver resolver = new ResultMappingConstructorResolver(configuration, mappings,
resultType, id);
resultMappings.addAll(resolver.resolveWithConstructor());
}

private Discriminator processDiscriminatorElement(XNode context, Class<?> resultType,
Expand Down
95 changes: 9 additions & 86 deletions src/main/java/org/apache/ibatis/mapping/ResultMap.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,13 @@
*/
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.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.ParamNameUtil;
import org.apache.ibatis.session.Configuration;

/**
Expand All @@ -55,8 +48,6 @@ private ResultMap() {
}

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

private final ResultMap resultMap = new ResultMap();

public Builder(Configuration configuration, String id, Class<?> type, List<ResultMapping> resultMappings) {
Expand Down Expand Up @@ -85,16 +76,18 @@ public ResultMap build() {
if (resultMap.id == null) {
throw new IllegalArgumentException("ResultMaps must have an id");
}

resultMap.mappedColumns = new HashSet<>();
resultMap.mappedProperties = new HashSet<>();
resultMap.idResultMappings = new ArrayList<>();
resultMap.constructorResultMappings = new ArrayList<>();
resultMap.propertyResultMappings = new ArrayList<>();
final List<String> constructorArgNames = new ArrayList<>();

for (ResultMapping resultMapping : resultMap.resultMappings) {
resultMap.hasNestedQueries = resultMap.hasNestedQueries || resultMapping.getNestedQueryId() != null;
resultMap.hasNestedResultMaps = resultMap.hasNestedResultMaps
|| resultMapping.getNestedResultMapId() != null && resultMapping.getResultSet() == null;

final String column = resultMapping.getColumn();
if (column != null) {
resultMap.mappedColumns.add(column.toUpperCase(Locale.ENGLISH));
Expand All @@ -106,10 +99,12 @@ 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);

Expand All @@ -118,99 +113,27 @@ public ResultMap build() {
resultMap.hasResultMapsUsingConstructorCollection = resultMap.hasResultMapsUsingConstructorCollection
|| (resultMapping.getNestedQueryId() == null && resultMapping.getTypeHandler() == null && javaType != null
&& resultMap.configuration.getObjectFactory().isCollection(javaType));

if (resultMapping.getProperty() != null) {
constructorArgNames.add(resultMapping.getProperty());
}
} else {
resultMap.propertyResultMappings.add(resultMapping);
}

if (resultMapping.getFlags().contains(ResultFlag.ID)) {
resultMap.idResultMappings.add(resultMapping);
}
}

if (resultMap.idResultMappings.isEmpty()) {
resultMap.idResultMappings.addAll(resultMap.resultMappings);
}
if (!constructorArgNames.isEmpty()) {
final List<String> actualArgNames = argNamesOfMatchingConstructor(constructorArgNames);
if (actualArgNames == null) {
throw new BuilderException("Error in result map '" + resultMap.id + "'. Failed to find a constructor in '"
+ resultMap.getType().getName() + "' with arg names " + constructorArgNames
+ ". Note that 'javaType' is required when there is no writable property with the same name ('name' is optional, BTW). There might be more info in debug log.");
}
resultMap.constructorResultMappings.sort((o1, o2) -> {
int paramIdx1 = actualArgNames.indexOf(o1.getProperty());
int paramIdx2 = actualArgNames.indexOf(o2.getProperty());
return paramIdx1 - paramIdx2;
});
}

// lock down collections
resultMap.resultMappings = Collections.unmodifiableList(resultMap.resultMappings);
resultMap.idResultMappings = Collections.unmodifiableList(resultMap.idResultMappings);
resultMap.constructorResultMappings = Collections.unmodifiableList(resultMap.constructorResultMappings);
resultMap.propertyResultMappings = Collections.unmodifiableList(resultMap.propertyResultMappings);
resultMap.mappedColumns = Collections.unmodifiableSet(resultMap.mappedColumns);
return resultMap;
}

private List<String> argNamesOfMatchingConstructor(List<String> constructorArgNames) {
Constructor<?>[] constructors = resultMap.type.getDeclaredConstructors();
for (Constructor<?> constructor : constructors) {
Class<?>[] paramTypes = constructor.getParameterTypes();
if (constructorArgNames.size() == paramTypes.length) {
List<String> paramNames = getArgNames(constructor);
if (constructorArgNames.containsAll(paramNames)
&& argTypesMatch(constructorArgNames, paramTypes, paramNames)) {
return paramNames;
}
}
}
return null;
}

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("While building result map '" + resultMap.id + "', 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) {
List<String> paramNames = new ArrayList<>();
List<String> actualParamNames = null;
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;
}
}
if (name == null && resultMap.configuration.isUseActualParamName()) {
if (actualParamNames == null) {
actualParamNames = ParamNameUtil.getParamNames(constructor);
}
if (actualParamNames.size() > paramIndex) {
name = actualParamNames.get(paramIndex);
}
}
paramNames.add(name != null ? name : "arg" + paramIndex);
}
return paramNames;
return resultMap;
}
}

Expand Down
Loading
Loading