From 400cdd94c12a3863f78ef3d6a96a970cfe7905e5 Mon Sep 17 00:00:00 2001 From: Willie Scholtz Date: Fri, 3 Jan 2025 17:41:17 +0100 Subject: [PATCH 01/12] #2618 When `javaType` has not (or partially) been specified, try to determine the best matching constructor - falls back to current behaviour if any ambiguity is detected - remove setters from immutable `ResultMapping` --- .../builder/MapperBuilderAssistant.java | 67 ++++++++++- .../annotation/MapperAnnotationBuilder.java | 9 +- .../ibatis/builder/xml/XMLMapperBuilder.java | 12 +- .../org/apache/ibatis/mapping/ResultMap.java | 18 +-- .../apache/ibatis/mapping/ResultMapping.java | 38 ++++--- .../apache/ibatis/session/Configuration.java | 28 ++--- .../Account.java | 20 ++++ .../Account2.java | 23 ++++ .../Account3.java | 27 +++++ ...toTypeFromNonAmbiguousConstructorTest.java | 106 ++++++++++++++++++ .../Mapper.java | 28 +++++ .../Mapper1.java | 35 ++++++ .../ibatis/immutable/ImmutableBlogMapper.xml | 12 +- .../CreateDB.sql | 26 +++++ .../Mapper.xml | 77 +++++++++++++ .../mybatis-config.xml | 42 +++++++ 16 files changed, 520 insertions(+), 48 deletions(-) create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account2.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account3.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java create mode 100644 src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/CreateDB.sql create mode 100644 src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml create mode 100644 src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config.xml diff --git a/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java b/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java index ac60fafab5b..52eee880c9c 100644 --- a/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java +++ b/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java @@ -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. @@ -15,8 +15,10 @@ */ package org.apache.ibatis.builder; +import java.lang.reflect.Constructor; import java.sql.ResultSet; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -25,6 +27,7 @@ import java.util.Properties; import java.util.Set; import java.util.StringTokenizer; +import java.util.stream.Collectors; import org.apache.ibatis.cache.Cache; import org.apache.ibatis.cache.decorators.LruCache; @@ -467,4 +470,66 @@ private Class resolveParameterJavaType(Class resultType, String property, return javaType; } + /** + * Attempts to assign a {@code javaType} to result mappings when it has been omitted, this is done based on matching + * constructors of the specified {@code resultType} + * + * @param resultType + * the result type of the object to be built + * @param resultMappings + * the current mappings + * + * @return null if there are no missing javaType mappings, or if no suitable mapping could be determined + */ + public List autoTypeResultMappingsForUnknownJavaTypes(Class resultType, + List resultMappings) { + // check if we have any undefined java types present, and try to set them automatically + if (resultMappings.stream().noneMatch(resultMapping -> Object.class.equals(resultMapping.getJavaType()))) { + return null; + } + + final List>> matchingConstructors = Arrays.stream(resultType.getDeclaredConstructors()) + .map(Constructor::getParameterTypes).filter(parameters -> parameters.length == resultMappings.size()) + .map(Arrays::asList).collect(Collectors.toList()); + + final List> typesToMatch = resultMappings.stream().map(ResultMapping::getJavaType) + .collect(Collectors.toList()); + + List> matchingTypes = null; + + outer: for (final List> actualTypes : matchingConstructors) { + for (int j = 0; j < typesToMatch.size(); j++) { + final Class type = typesToMatch.get(j); + // pre-filled a type, check if it matches the constructor + if (!Object.class.equals(type) && !type.equals(actualTypes.get(j))) { + continue outer; + } + } + + if (matchingTypes != null) { + // multiple matches found, abort as we cannot reliably guess the correct one. + matchingTypes = null; + break; + } + + matchingTypes = actualTypes; + } + + if (matchingTypes == null) { + return null; + } + + final List adjustedAutoTypeResultMappings = new ArrayList<>(); + for (int i = 0; i < resultMappings.size(); i++) { + ResultMapping otherMapping = resultMappings.get(i); + Class identifiedMatchingJavaType = matchingTypes.get(i); + + // given that we selected a new java type, overwrite the currently + // selected type handler so it can get retrieved again from the registry + adjustedAutoTypeResultMappings + .add(new ResultMapping.Builder(otherMapping).javaType(identifiedMatchingJavaType).typeHandler(null).build()); + } + + return adjustedAutoTypeResultMappings; + } } diff --git a/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java b/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java index 0388256e5e7..50004f29a39 100644 --- a/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java +++ b/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java @@ -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. @@ -29,6 +29,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Properties; import java.util.Set; @@ -514,6 +515,7 @@ private boolean hasNestedSelect(Result result) { } private void applyConstructorArgs(Arg[] args, Class resultType, List resultMappings) { + final List mappings = new ArrayList<>(); for (Arg arg : args) { List flags = new ArrayList<>(); flags.add(ResultFlag.CONSTRUCTOR); @@ -527,8 +529,11 @@ private void applyConstructorArgs(Arg[] args, Class resultType, List autoMappings = assistant.autoTypeResultMappingsForUnknownJavaTypes(resultType, mappings); + resultMappings.addAll(Objects.requireNonNullElse(autoMappings, mappings)); } private String nullOrEmpty(String value) { diff --git a/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java b/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java index 069cb82ba55..2c8cf023c97 100644 --- a/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java +++ b/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java @@ -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. @@ -23,6 +23,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Properties; import org.apache.ibatis.builder.BaseBuilder; @@ -267,14 +268,21 @@ protected Class inheritEnclosingType(XNode resultMapNode, Class enclosingT private void processConstructorElement(XNode resultChild, Class resultType, List resultMappings) { List argChildren = resultChild.getChildren(); + + final List mappings = new ArrayList<>(); for (XNode argChild : argChildren) { List 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 List autoMappings = builderAssistant.autoTypeResultMappingsForUnknownJavaTypes(resultType, + mappings); + resultMappings.addAll(Objects.requireNonNullElse(autoMappings, mappings)); } private Discriminator processDiscriminatorElement(XNode context, Class resultType, diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMap.java b/src/main/java/org/apache/ibatis/mapping/ResultMap.java index 58ce5dd6f6a..67ca2ae33de 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMap.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMap.java @@ -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. @@ -15,6 +15,13 @@ */ package org.apache.ibatis.mapping; +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; + import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; import java.util.ArrayList; @@ -24,13 +31,6 @@ 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; - /** * @author Clinton Begin */ @@ -137,7 +137,7 @@ public ResultMap build() { 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."); + + ". Note that 'javaType' is required when there is ambiguous constructors or 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()); diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java index 9ec710679ca..e5314a4da3b 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java @@ -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. @@ -15,16 +15,16 @@ */ package org.apache.ibatis.mapping; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Set; - import org.apache.ibatis.session.Configuration; import org.apache.ibatis.type.JdbcType; import org.apache.ibatis.type.TypeHandler; import org.apache.ibatis.type.TypeHandlerRegistry; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Set; + /** * @author Clinton Begin */ @@ -72,6 +72,24 @@ public Builder(Configuration configuration, String property) { resultMapping.lazy = configuration.isLazyLoadingEnabled(); } + public Builder(ResultMapping otherMapping) { + this(otherMapping.configuration, otherMapping.property); + + resultMapping.flags.addAll(otherMapping.flags); + resultMapping.composites.addAll(otherMapping.composites); + + resultMapping.column = otherMapping.column; + resultMapping.javaType = otherMapping.javaType; + resultMapping.jdbcType = otherMapping.jdbcType; + resultMapping.typeHandler = otherMapping.typeHandler; + resultMapping.nestedResultMapId = otherMapping.nestedResultMapId; + resultMapping.nestedQueryId = otherMapping.nestedQueryId; + resultMapping.notNullColumns = otherMapping.notNullColumns; + resultMapping.columnPrefix = otherMapping.columnPrefix; + resultMapping.resultSet = otherMapping.resultSet; + resultMapping.foreignColumn = otherMapping.foreignColumn; + } + public Builder javaType(Class javaType) { resultMapping.javaType = javaType; return this; @@ -243,18 +261,10 @@ public String getForeignColumn() { return foreignColumn; } - public void setForeignColumn(String foreignColumn) { - this.foreignColumn = foreignColumn; - } - public boolean isLazy() { return lazy; } - public void setLazy(boolean lazy) { - this.lazy = lazy; - } - public boolean isSimple() { return this.nestedResultMapId == null && this.nestedQueryId == null && this.resultSet == null; } diff --git a/src/main/java/org/apache/ibatis/session/Configuration.java b/src/main/java/org/apache/ibatis/session/Configuration.java index 8234250a6ed..d610254f28d 100644 --- a/src/main/java/org/apache/ibatis/session/Configuration.java +++ b/src/main/java/org/apache/ibatis/session/Configuration.java @@ -15,6 +15,20 @@ */ package org.apache.ibatis.session; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantLock; +import java.util.function.BiFunction; + import org.apache.ibatis.binding.MapperRegistry; import org.apache.ibatis.builder.CacheRefResolver; import org.apache.ibatis.builder.IncompleteElementException; @@ -83,20 +97,6 @@ import org.apache.ibatis.type.TypeHandler; import org.apache.ibatis.type.TypeHandlerRegistry; -import java.util.Arrays; -import java.util.Collection; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Iterator; -import java.util.LinkedList; -import java.util.List; -import java.util.Map; -import java.util.Properties; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.locks.ReentrantLock; -import java.util.function.BiFunction; - /** * @author Clinton Begin */ diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account.java new file mode 100644 index 00000000000..40905ee1755 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account.java @@ -0,0 +1,20 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +public record Account(long accountId, String accountName, String accountType) { + +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account2.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account2.java new file mode 100644 index 00000000000..c7664104ad5 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account2.java @@ -0,0 +1,23 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +public record Account2(long accountId, String accountName, String accountType) { + + public Account2(long accountId, String accountName, String accountType, String extraInfo) { + this(accountId, accountName, accountType); + } +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account3.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account3.java new file mode 100644 index 00000000000..17184147e97 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account3.java @@ -0,0 +1,27 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +public record Account3(long accountId, String accountName, String accountType) { + + public Account3(long accountId, int mismatch, String accountType) { + this(accountId, "MismatchedAccountI", accountType); + } + + public Account3(long accountId, long mismatch, String accountType) { + this(accountId, "MismatchedAccountL", accountType); + } +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java new file mode 100644 index 00000000000..f7476b0e498 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java @@ -0,0 +1,106 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +import java.io.Reader; + +import org.apache.ibatis.BaseDataTest; +import org.apache.ibatis.io.Resources; +import org.apache.ibatis.session.SqlSession; +import org.apache.ibatis.session.SqlSessionFactory; +import org.apache.ibatis.session.SqlSessionFactoryBuilder; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +class AutoTypeFromNonAmbiguousConstructorTest { + + private static SqlSessionFactory sqlSessionFactory; + + @BeforeAll + static void setUp() throws Exception { + try (Reader reader = Resources.getResourceAsReader( + "org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config.xml")) { + sqlSessionFactory = new SqlSessionFactoryBuilder().build(reader); + } + + BaseDataTest.runScript(sqlSessionFactory.getConfiguration().getEnvironment().getDataSource(), + "org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/CreateDB.sql"); + } + + @Test + void testNormalCaseWhereAllTypesAreProvided() { + try (SqlSession sqlSession = sqlSessionFactory.openSession()) { + Mapper mapper = sqlSession.getMapper(Mapper.class); + Account account = mapper.getAccountNonAmbiguous(1); + Assertions.assertThat(account).isNotNull(); + Assertions.assertThat(account.accountId()).isEqualTo(1); + Assertions.assertThat(account.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account.accountType()).isEqualTo("Current"); + } + } + + @Test + void testNoTypesAreProvidedAndUsesAutoType() { + try (SqlSession sqlSession = sqlSessionFactory.openSession()) { + Mapper mapper = sqlSession.getMapper(Mapper.class); + Account account = mapper.getAccountJavaTypesMissing(1); + Assertions.assertThat(account).isNotNull(); + Assertions.assertThat(account.accountId()).isEqualTo(1); + Assertions.assertThat(account.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account.accountType()).isEqualTo("Current"); + + Mapper1 mapper1 = sqlSession.getMapper(Mapper1.class); + Account account1 = mapper1.getAccountJavaTypesMissing(1); + Assertions.assertThat(account1).isNotNull(); + Assertions.assertThat(account1.accountId()).isEqualTo(1); + Assertions.assertThat(account1.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account1.accountType()).isEqualTo("Current"); + } + } + + @Test + void testSucceedsWhenConstructorWithMoreTypesAreFound() { + try (SqlSession sqlSession = sqlSessionFactory.openSession()) { + Mapper mapper = sqlSession.getMapper(Mapper.class); + Account2 account = mapper.getAccountExtraParameter(1); + Assertions.assertThat(account).isNotNull(); + Assertions.assertThat(account.accountId()).isEqualTo(1); + Assertions.assertThat(account.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account.accountType()).isEqualTo("Current"); + } + } + + @Test + void testChoosesCorrectConstructorWhenPartialTypesAreProvided() { + try (SqlSession sqlSession = sqlSessionFactory.openSession()) { + Mapper mapper = sqlSession.getMapper(Mapper.class); + Account3 account = mapper.getAccountPartialTypesProvided(1); + Assertions.assertThat(account).isNotNull(); + Assertions.assertThat(account.accountId()).isEqualTo(1); + Assertions.assertThat(account.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account.accountType()).isEqualTo("Current"); + + Mapper1 mapper1 = sqlSession.getMapper(Mapper1.class); + Account3 account1 = mapper1.getAccountPartialTypesProvided(1); + Assertions.assertThat(account1).isNotNull(); + Assertions.assertThat(account1.accountId()).isEqualTo(1); + Assertions.assertThat(account1.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account1.accountType()).isEqualTo("Current"); + } + } + +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java new file mode 100644 index 00000000000..36d33ee3a8c --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java @@ -0,0 +1,28 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +public interface Mapper { + + Account getAccountNonAmbiguous(long id); + + Account getAccountJavaTypesMissing(long id); + + Account2 getAccountExtraParameter(long id); + + Account3 getAccountPartialTypesProvided(long id); + +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java new file mode 100644 index 00000000000..b0575544754 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java @@ -0,0 +1,35 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +import org.apache.ibatis.annotations.Arg; +import org.apache.ibatis.annotations.ConstructorArgs; +import org.apache.ibatis.annotations.Mapper; +import org.apache.ibatis.annotations.Select; + +@Mapper +public interface Mapper1 { + + String SELECT_SQL = "select a.id, a.name, a.type from account a where a.id = #{id}"; + + @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name"), @Arg(column = "type"), }) + @Select(SELECT_SQL) + Account getAccountJavaTypesMissing(long id); + + @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name", javaType = String.class), @Arg(column = "type"), }) + @Select(SELECT_SQL) + Account3 getAccountPartialTypesProvided(long id); +} diff --git a/src/test/resources/org/apache/ibatis/immutable/ImmutableBlogMapper.xml b/src/test/resources/org/apache/ibatis/immutable/ImmutableBlogMapper.xml index 41c8f3f56d3..b3989f00cb6 100644 --- a/src/test/resources/org/apache/ibatis/immutable/ImmutableBlogMapper.xml +++ b/src/test/resources/org/apache/ibatis/immutable/ImmutableBlogMapper.xml @@ -1,7 +1,7 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + select a.id + , a.name + , a.type + from account a + where a.id = #{id} + + + + + + + + + + \ No newline at end of file diff --git a/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config.xml b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config.xml new file mode 100644 index 00000000000..0b9e0c18dc8 --- /dev/null +++ b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config.xml @@ -0,0 +1,42 @@ + + + + + + + + + + + + + + + + + + + + + + + From 0ebdcd28e305ab6ebf7ba93024fc5b7187a83cb3 Mon Sep 17 00:00:00 2001 From: Willie Scholtz Date: Fri, 3 Jan 2025 17:48:24 +0100 Subject: [PATCH 02/12] Running impsort --- .../java/org/apache/ibatis/mapping/ResultMap.java | 14 +++++++------- .../org/apache/ibatis/mapping/ResultMapping.java | 10 +++++----- .../ConfigurationTest.java | 8 ++++---- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMap.java b/src/main/java/org/apache/ibatis/mapping/ResultMap.java index 67ca2ae33de..ed16f34b8c9 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMap.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMap.java @@ -15,13 +15,6 @@ */ package org.apache.ibatis.mapping; -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; - import java.lang.annotation.Annotation; import java.lang.reflect.Constructor; import java.util.ArrayList; @@ -31,6 +24,13 @@ 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; + /** * @author Clinton Begin */ diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java index e5314a4da3b..47c09f90400 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java @@ -15,16 +15,16 @@ */ package org.apache.ibatis.mapping; -import org.apache.ibatis.session.Configuration; -import org.apache.ibatis.type.JdbcType; -import org.apache.ibatis.type.TypeHandler; -import org.apache.ibatis.type.TypeHandlerRegistry; - import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Set; +import org.apache.ibatis.session.Configuration; +import org.apache.ibatis.type.JdbcType; +import org.apache.ibatis.type.TypeHandler; +import org.apache.ibatis.type.TypeHandlerRegistry; + /** * @author Clinton Begin */ diff --git a/src/test/java/org/apache/ibatis/submitted/global_variables_defaults/ConfigurationTest.java b/src/test/java/org/apache/ibatis/submitted/global_variables_defaults/ConfigurationTest.java index 1550979147c..91c1e181415 100644 --- a/src/test/java/org/apache/ibatis/submitted/global_variables_defaults/ConfigurationTest.java +++ b/src/test/java/org/apache/ibatis/submitted/global_variables_defaults/ConfigurationTest.java @@ -15,6 +15,10 @@ */ package org.apache.ibatis.submitted.global_variables_defaults; +import java.io.IOException; +import java.io.Reader; +import java.util.Properties; + import org.apache.ibatis.builder.StaticSqlSource; import org.apache.ibatis.datasource.unpooled.UnpooledDataSource; import org.apache.ibatis.io.Resources; @@ -28,10 +32,6 @@ import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; -import java.io.IOException; -import java.io.Reader; -import java.util.Properties; - class ConfigurationTest { @Test From 541fc61e6318596e39b21b1acb0fbaf18479964f Mon Sep 17 00:00:00 2001 From: Willie Scholtz Date: Sat, 4 Jan 2025 09:47:47 +0100 Subject: [PATCH 03/12] Added coverage for failing cases - Removed label outer: structure --- .../builder/MapperBuilderAssistant.java | 24 +++++++---- .../Account1.java | 22 ++++++++++ ...romNonAmbiguousConstructorFailingTest.java | 39 ++++++++++++++++++ .../FailingMapper.java | 22 ++++++++++ .../FailingMapper.xml | 41 +++++++++++++++++++ .../mybatis-config-failing.xml | 41 +++++++++++++++++++ 6 files changed, 181 insertions(+), 8 deletions(-) create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorFailingTest.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.java create mode 100644 src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.xml create mode 100644 src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config-failing.xml diff --git a/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java b/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java index 52eee880c9c..55069b79848 100644 --- a/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java +++ b/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java @@ -480,11 +480,16 @@ private Class resolveParameterJavaType(Class resultType, String property, * the current mappings * * @return null if there are no missing javaType mappings, or if no suitable mapping could be determined + * + * @see #2618 */ public List autoTypeResultMappingsForUnknownJavaTypes(Class resultType, List resultMappings) { + final List> typesToMatch = resultMappings.stream().map(ResultMapping::getJavaType) + .collect(Collectors.toList()); + // check if we have any undefined java types present, and try to set them automatically - if (resultMappings.stream().noneMatch(resultMapping -> Object.class.equals(resultMapping.getJavaType()))) { + if (typesToMatch.stream().noneMatch(type -> type == null || Object.class.equals(type))) { return null; } @@ -492,20 +497,23 @@ public List autoTypeResultMappingsForUnknownJavaTypes(Class re .map(Constructor::getParameterTypes).filter(parameters -> parameters.length == resultMappings.size()) .map(Arrays::asList).collect(Collectors.toList()); - final List> typesToMatch = resultMappings.stream().map(ResultMapping::getJavaType) - .collect(Collectors.toList()); - List> matchingTypes = null; - - outer: for (final List> actualTypes : matchingConstructors) { + for (List> actualTypes : matchingConstructors) { + boolean matchesType = true; for (int j = 0; j < typesToMatch.size(); j++) { final Class type = typesToMatch.get(j); + // pre-filled a type, check if it matches the constructor - if (!Object.class.equals(type) && !type.equals(actualTypes.get(j))) { - continue outer; + if (type != null && !Object.class.equals(type) && !type.equals(actualTypes.get(j))) { + matchesType = false; + break; } } + if (!matchesType) { + continue; + } + if (matchingTypes != null) { // multiple matches found, abort as we cannot reliably guess the correct one. matchingTypes = null; diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java new file mode 100644 index 00000000000..1480b2beeb2 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java @@ -0,0 +1,22 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +public record Account1(long accountId, String accountName, String accountType) { + public Account1(long accountId, String accountName, int accountTypeId) { + this(accountId, accountName, String.valueOf(accountTypeId)); + } +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorFailingTest.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorFailingTest.java new file mode 100644 index 00000000000..f6cb3d612e5 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorFailingTest.java @@ -0,0 +1,39 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +import java.io.IOException; +import java.io.Reader; + +import org.apache.ibatis.builder.BuilderException; +import org.apache.ibatis.io.Resources; +import org.apache.ibatis.session.SqlSessionFactoryBuilder; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Test; + +class AutoTypeFromNonAmbiguousConstructorFailingTest { + + @Test + void testCannotResolveAmbiguousConstructor() throws IOException { + // Account1 has more than 1 matching constructor, and auto type cannot decide which one to use + try (Reader reader = Resources.getResourceAsReader( + "org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config-failing.xml")) { + + Assertions.assertThatThrownBy(() -> new SqlSessionFactoryBuilder().build(reader)).isNotNull() + .hasCauseInstanceOf(BuilderException.class).hasMessageContaining("Failed to find a constructor"); + } + } +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.java new file mode 100644 index 00000000000..19903294978 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.java @@ -0,0 +1,22 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +public interface FailingMapper { + + Account1 getAccountAmbiguous(long id); + +} diff --git a/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.xml b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.xml new file mode 100644 index 00000000000..db191a9cfb4 --- /dev/null +++ b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/FailingMapper.xml @@ -0,0 +1,41 @@ + + + + + + + + + + + + + + + select a.id + , a.name + , a.type + from account a + where a.id = #{id} + + + + \ No newline at end of file diff --git a/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config-failing.xml b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config-failing.xml new file mode 100644 index 00000000000..3aec2aeb62c --- /dev/null +++ b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/mybatis-config-failing.xml @@ -0,0 +1,41 @@ + + + + + + + + + + + + + + + + + + + + + + From 6b12edc214f2f483513c8e6611f7ad719f92f781 Mon Sep 17 00:00:00 2001 From: Willie Scholtz Date: Mon, 6 Jan 2025 16:00:30 +0100 Subject: [PATCH 04/12] feat: Centralise constructor resolver --- .../builder/MapperBuilderAssistant.java | 74 ---- .../ResultMappingConstructorResolver.java | 367 ++++++++++++++++++ .../annotation/MapperAnnotationBuilder.java | 14 +- .../ibatis/builder/xml/XMLMapperBuilder.java | 22 +- .../org/apache/ibatis/mapping/ResultMap.java | 91 +---- .../ResultMappingConstructorResolverTest.java | 345 ++++++++++++++++ .../Account1.java | 4 +- .../Account4.java | 21 + ...toTypeFromNonAmbiguousConstructorTest.java | 18 + .../Mapper.java | 1 + .../Mapper1.java | 13 +- .../Mapper.xml | 13 + 12 files changed, 808 insertions(+), 175 deletions(-) create mode 100644 src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java create mode 100644 src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java create mode 100644 src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account4.java diff --git a/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java b/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java index 55069b79848..4067e81d899 100644 --- a/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java +++ b/src/main/java/org/apache/ibatis/builder/MapperBuilderAssistant.java @@ -15,10 +15,8 @@ */ package org.apache.ibatis.builder; -import java.lang.reflect.Constructor; import java.sql.ResultSet; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -27,7 +25,6 @@ import java.util.Properties; import java.util.Set; import java.util.StringTokenizer; -import java.util.stream.Collectors; import org.apache.ibatis.cache.Cache; import org.apache.ibatis.cache.decorators.LruCache; @@ -469,75 +466,4 @@ private Class resolveParameterJavaType(Class resultType, String property, } return javaType; } - - /** - * Attempts to assign a {@code javaType} to result mappings when it has been omitted, this is done based on matching - * constructors of the specified {@code resultType} - * - * @param resultType - * the result type of the object to be built - * @param resultMappings - * the current mappings - * - * @return null if there are no missing javaType mappings, or if no suitable mapping could be determined - * - * @see #2618 - */ - public List autoTypeResultMappingsForUnknownJavaTypes(Class resultType, - List resultMappings) { - final List> typesToMatch = resultMappings.stream().map(ResultMapping::getJavaType) - .collect(Collectors.toList()); - - // check if we have any undefined java types present, and try to set them automatically - if (typesToMatch.stream().noneMatch(type -> type == null || Object.class.equals(type))) { - return null; - } - - final List>> matchingConstructors = Arrays.stream(resultType.getDeclaredConstructors()) - .map(Constructor::getParameterTypes).filter(parameters -> parameters.length == resultMappings.size()) - .map(Arrays::asList).collect(Collectors.toList()); - - List> matchingTypes = null; - for (List> actualTypes : matchingConstructors) { - boolean matchesType = true; - for (int j = 0; j < typesToMatch.size(); j++) { - final Class type = typesToMatch.get(j); - - // pre-filled a type, check if it matches the constructor - if (type != null && !Object.class.equals(type) && !type.equals(actualTypes.get(j))) { - matchesType = false; - break; - } - } - - if (!matchesType) { - continue; - } - - if (matchingTypes != null) { - // multiple matches found, abort as we cannot reliably guess the correct one. - matchingTypes = null; - break; - } - - matchingTypes = actualTypes; - } - - if (matchingTypes == null) { - return null; - } - - final List adjustedAutoTypeResultMappings = new ArrayList<>(); - for (int i = 0; i < resultMappings.size(); i++) { - ResultMapping otherMapping = resultMappings.get(i); - Class identifiedMatchingJavaType = matchingTypes.get(i); - - // given that we selected a new java type, overwrite the currently - // selected type handler so it can get retrieved again from the registry - adjustedAutoTypeResultMappings - .add(new ResultMapping.Builder(otherMapping).javaType(identifiedMatchingJavaType).typeHandler(null).build()); - } - - return adjustedAutoTypeResultMappings; - } } diff --git a/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java new file mode 100644 index 00000000000..4fe821fc170 --- /dev/null +++ b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java @@ -0,0 +1,367 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.builder; + +import java.lang.annotation.Annotation; +import java.lang.reflect.Constructor; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; + +import org.apache.ibatis.annotations.Param; +import org.apache.ibatis.logging.Log; +import org.apache.ibatis.logging.LogFactory; +import org.apache.ibatis.mapping.ResultMapping; +import org.apache.ibatis.reflection.ParamNameUtil; +import org.apache.ibatis.session.Configuration; +import org.apache.ibatis.type.TypeHandler; +import org.apache.ibatis.type.UnknownTypeHandler; + +public class ResultMappingConstructorResolver { + + private static final Log log = LogFactory.getLog(ResultMappingConstructorResolver.class); + + private final Configuration configuration; + private final List constructorResultMappings; + private final Class resultType; + private final String resultMapId; + + /** + * @param configuration + * the global configuration object + * @param constructorResultMappings + * the current mappings as resolved from xml or annotations + * @param resultType + * the result type of the object to be built + */ + public ResultMappingConstructorResolver(Configuration configuration, List constructorResultMappings, + Class resultType, String resultMapId) { + this.configuration = configuration; + this.constructorResultMappings = Objects.requireNonNull(constructorResultMappings); + this.resultType = Objects.requireNonNull(resultType); + this.resultMapId = resultMapId; + } + + /** + * Attempts to find a matching constructor for the supplied {@code resultType} and (possibly unordered mappings) by: + *
    + *
  1. Finding constructors with the same amount of arguments
    2. + *
  2. Rejecting candidates which have different argument names than our mappings
    3. + *
  3. Ensuring the type of each mapping matches the resolved constructor
    4. + *
  4. Rebuilding and sorting the mappings according to the found constructor
    5. + *
+ *

+ * Note that if there are multiple constructors which match, {@code javaType} is the only way to differentiate between + * them. i.e. if there is only one constructor, all types could be missing, however if there is more than one with the + * same amount of arguments, we need type on at least a few mappings to differentiate and select the correct + * constructor. + *

+ * Argument order can only be derived if all mappings have a {@code name} specified, as this is the only way we order + * against the constructor reliably, i.e. we cannot order {@code X(String, String, String)} based on types alone. + * + * @return ordered mappings based on the resolved constructor, original mappings if none were found, or original + * mappings if they did not have property names and a constructor could not be resolved + * + * @throws BuilderException + * when a constructor could not be resolved + * + * @see #2618 + * @see #721 + */ + public List resolveWithConstructor() { + if (constructorResultMappings.isEmpty()) { + // todo: AutoMapping works during runtime, we cannot resolve constructors yet + return constructorResultMappings; + } + + // retrieve constructors & trim selection down to parameter length + final List matchingConstructorCandidates = retrieveConstructorCandidates( + constructorResultMappings.size()); + + // extract the property names we have + final Set constructorArgsByName = constructorResultMappings.stream().map(ResultMapping::getProperty) + .filter(Objects::nonNull).collect(Collectors.toCollection(LinkedHashSet::new)); + + // arg order can only be 'fixed' if all mappings have property names + final boolean allMappingsHavePropertyNames = constructorResultMappings.size() == constructorArgsByName.size(); + + // only do this if all property mappings were set + if (allMappingsHavePropertyNames) { + // while we have candidates, start selection + removeCandidatesBasedOnParameterNames(matchingConstructorCandidates, constructorArgsByName); + } + + // resolve final constructor by filtering out selection based on type info present (or missing) + final ConstructorMetaInfo matchingConstructorInfo = filterBasedOnType(matchingConstructorCandidates, + constructorResultMappings, allMappingsHavePropertyNames); + if (matchingConstructorInfo == null) { + // [backwards-compatibility] (we cannot find a constructor), + // but this used to get thrown ONLY when property mappings have been set + if (allMappingsHavePropertyNames) { + throw new BuilderException("Error in result map '" + resultMapId + "'. Failed to find a constructor in '" + + resultType.getName() + "' with arg names " + constructorArgsByName + + ". Note that 'javaType' is required when there is ambiguous constructors or there is no writable property with the same name ('name' is optional, BTW). There is more info in the debug log."); + } else { + if (log.isDebugEnabled()) { + log.debug("Constructor for '" + resultMapId + + "' could not be resolved, continuing, but this may result in a mapping exception later"); + } + // return un-modified original mappings (maybe have this as a config flag, as this will result in a runtime + // exception eventually + return constructorResultMappings; + } + } + + // only rebuild (auto-type) if required (any types are unidentified) + final boolean autoTypeRequired = constructorResultMappings.stream().map(ResultMapping::getJavaType) + .anyMatch(mappingType -> mappingType == null || Object.class.equals(mappingType)); + final List resultMappings = autoTypeRequired + ? autoTypeConstructorMappings(matchingConstructorInfo, constructorResultMappings, allMappingsHavePropertyNames) + : constructorResultMappings; + + if (allMappingsHavePropertyNames) { + // finally sort them based on the constructor meta info + sortConstructorMappings(matchingConstructorInfo, resultMappings); + } + + return resultMappings; + } + + List retrieveConstructorCandidates(int withLength) { + return Arrays.stream(resultType.getDeclaredConstructors()) + .filter(constructor -> constructor.getParameterTypes().length == withLength).map(ConstructorMetaInfo::new) + .collect(Collectors.toList()); + } + + private static void removeCandidatesBasedOnParameterNames(List matchingConstructorCandidates, + Set constructorArgsByName) { + final Iterator candidateIterator = matchingConstructorCandidates.iterator(); + while (candidateIterator.hasNext()) { + // extract the names (and types) the constructor has + final ConstructorMetaInfo candidateInfo = candidateIterator.next(); + + // if all our param names contain all the derived names, keep candidate + if (!candidateInfo.isApplicableFor(constructorArgsByName)) { + if (log.isDebugEnabled()) { + log.debug("While resolving the constructor '" + candidateInfo + "', it was excluded from selection. " + + "' Required parameters: [" + constructorArgsByName + "] Actual: [" + + candidateInfo.constructorArgs.keySet() + "]"); + } + + candidateIterator.remove(); + } + } + } + + private static ConstructorMetaInfo filterBasedOnType(List matchingConstructorCandidates, + List resultMappings, boolean allMappingsHavePropertyNames) { + ConstructorMetaInfo matchingConstructorInfo = null; + for (ConstructorMetaInfo constructorMetaInfo : matchingConstructorCandidates) { + boolean matchesType = true; + + for (int i = 0; i < resultMappings.size(); i++) { + final ResultMapping constructorMapping = resultMappings.get(i); + final Class type = constructorMapping.getJavaType(); + final ConstructorArg matchingArg = allMappingsHavePropertyNames + ? constructorMetaInfo.getArgByPropertyName(constructorMapping.getProperty()) + : constructorMetaInfo.getArgByOriginalIndex(i); + + if (matchingArg == null) { + if (log.isDebugEnabled()) { + log.debug("While resolving the constructor '" + constructorMetaInfo + "', it was excluded from selection. " + + "' Could not find constructor argument for mapping: [" + constructorMapping + "], available [" + + constructorMetaInfo.constructorArgs + "]"); + } + + matchesType = false; + break; + } + + // pre-filled a type, check if it matches the constructor + if (type != null && !Object.class.equals(type) && !type.equals(matchingArg.getType())) { + if (log.isDebugEnabled()) { + log.debug("While resolving the constructor '" + constructorMetaInfo + "', it was excluded from selection. " + + "' Required mapping: [" + constructorMapping + "] does not match actual type: [" + matchingArg + "]"); + } + + matchesType = false; + break; + } + } + + if (!matchesType) { + continue; + } + + if (matchingConstructorInfo != null) { + if (log.isDebugEnabled()) { + log.debug("While resolving the constructor '" + constructorMetaInfo + "', it was excluded from selection. " + + "Match already found! Ambiguous constructors [" + matchingConstructorInfo + "]"); + } + + // multiple matches found, abort as we cannot reliably guess the correct one. + matchingConstructorInfo = null; + break; + } + + matchingConstructorInfo = constructorMetaInfo; + } + + return matchingConstructorInfo; + } + + private List autoTypeConstructorMappings(ConstructorMetaInfo matchingConstructorInfo, + List resultMappings, boolean allMappingsHavePropertyNames) { + final List adjustedAutoTypeResultMappings = new ArrayList<>(constructorResultMappings.size()); + for (int i = 0; i < resultMappings.size(); i++) { + final ResultMapping originalMapping = resultMappings.get(i); + final ConstructorArg matchingArg = allMappingsHavePropertyNames + ? matchingConstructorInfo.getArgByPropertyName(originalMapping.getProperty()) + : matchingConstructorInfo.getArgByOriginalIndex(i); + + final TypeHandler originalTypeHandler = originalMapping.getTypeHandler(); + final TypeHandler typeHandler = originalTypeHandler == null + || originalTypeHandler.getClass().isAssignableFrom(UnknownTypeHandler.class) ? null : originalTypeHandler; + + // given that we selected a new java type, overwrite the currently + // selected type handler so it can get retrieved again from the registry + adjustedAutoTypeResultMappings.add( + new ResultMapping.Builder(originalMapping).javaType(matchingArg.getType()).typeHandler(typeHandler).build()); + } + + return adjustedAutoTypeResultMappings; + } + + private static void sortConstructorMappings(ConstructorMetaInfo matchingConstructorInfo, + List resultMappings) { + final List orderedConstructorParameters = new ArrayList<>(matchingConstructorInfo.constructorArgs.keySet()); + resultMappings.sort((o1, o2) -> { + int paramIdx1 = orderedConstructorParameters.indexOf(o1.getProperty()); + int paramIdx2 = orderedConstructorParameters.indexOf(o2.getProperty()); + return paramIdx1 - paramIdx2; + }); + } + + /** + * Represents a {@link Constructor} with parameter names and types + */ + class ConstructorMetaInfo { + + final Map constructorArgs; + final List argsByIndex; + + private ConstructorMetaInfo(Constructor constructor) { + final List args = fromConstructor(constructor); + + this.constructorArgs = args.stream() + .collect(Collectors.toMap(ConstructorArg::getName, arg -> arg, (arg1, arg2) -> arg1, LinkedHashMap::new)); + this.argsByIndex = new ArrayList<>(this.constructorArgs.values()); + } + + boolean isApplicableFor(Set resultMappingProperties) { + return resultMappingProperties.containsAll(constructorArgs.keySet()); + } + + ConstructorArg getArgByPropertyName(String name) { + return constructorArgs.get(name); + } + + ConstructorArg getArgByOriginalIndex(int index) { + if (argsByIndex.isEmpty() || index >= argsByIndex.size()) { + return null; + } + + return argsByIndex.get(index); + } + + private List fromConstructor(Constructor constructor) { + final Class[] parameterTypes = constructor.getParameterTypes(); + final List argNames = getArgNames(constructor); + + final List constructorArgs = new ArrayList<>(argNames.size()); + for (int i = 0; i < argNames.size(); i++) { + constructorArgs.add(new ConstructorArg(parameterTypes[i], argNames.get(i))); + } + + return constructorArgs; + } + + private List getArgNames(Constructor constructor) { + List paramNames = new ArrayList<>(); + List 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 && 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; + } + + @Override + public String toString() { + return "ConstructorMetaInfo{" + "args=" + constructorArgs + '}'; + } + } + + static class ConstructorArg { + private final Class type; + private final String name; + + private ConstructorArg(Class type, String name) { + this.type = type; + this.name = name; + } + + public Class getType() { + return type; + } + + public String getName() { + return name; + } + + @Override + public String toString() { + return "arg{" + "type=" + type.getName() + ", name='" + name + '\'' + '}'; + } + } +} diff --git a/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java b/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java index 50004f29a39..d9b349ab7c1 100644 --- a/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java +++ b/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java @@ -29,7 +29,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; import java.util.Properties; import java.util.Set; @@ -64,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; @@ -238,7 +238,7 @@ private String generateResultMapName(Method method) { private void applyResultMap(String resultMapId, Class returnType, Arg[] args, Result[] results, TypeDiscriminator discriminator) { List 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 @@ -252,7 +252,7 @@ private void createDiscriminatorResultMaps(String resultMapId, Class resultTy String caseResultMapId = resultMapId + "-" + c.value(); List 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); @@ -514,7 +514,8 @@ private boolean hasNestedSelect(Result result) { return result.one().select().length() > 0 || result.many().select().length() > 0; } - private void applyConstructorArgs(Arg[] args, Class resultType, List resultMappings) { + private void applyConstructorArgs(Arg[] args, Class resultType, List resultMappings, + String resultMapId) { final List mappings = new ArrayList<>(); for (Arg arg : args) { List flags = new ArrayList<>(); @@ -532,8 +533,9 @@ private void applyConstructorArgs(Arg[] args, Class resultType, List autoMappings = assistant.autoTypeResultMappingsForUnknownJavaTypes(resultType, mappings); - resultMappings.addAll(Objects.requireNonNullElse(autoMappings, mappings)); + final ResultMappingConstructorResolver resolver = new ResultMappingConstructorResolver(configuration, mappings, + resultType, resultMapId); + resultMappings.addAll(resolver.resolveWithConstructor()); } private String nullOrEmpty(String value) { diff --git a/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java b/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java index 2c8cf023c97..98e9c221c6b 100644 --- a/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java +++ b/src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java @@ -23,7 +23,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Properties; import org.apache.ibatis.builder.BaseBuilder; @@ -32,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; @@ -224,12 +224,17 @@ private ResultMap resultMapElement(XNode resultMapNode, List 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 resultMappings = new ArrayList<>(additionalResultMappings); List 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 { @@ -240,9 +245,7 @@ private ResultMap resultMapElement(XNode resultMapNode, List 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 { @@ -266,7 +269,8 @@ protected Class inheritEnclosingType(XNode resultMapNode, Class enclosingT return null; } - private void processConstructorElement(XNode resultChild, Class resultType, List resultMappings) { + private void processConstructorElement(XNode resultChild, Class resultType, List resultMappings, + String id) { List argChildren = resultChild.getChildren(); final List mappings = new ArrayList<>(); @@ -280,9 +284,9 @@ private void processConstructorElement(XNode resultChild, Class resultType, L mappings.add(buildResultMappingFromContext(argChild, resultType, flags)); } - final List autoMappings = builderAssistant.autoTypeResultMappingsForUnknownJavaTypes(resultType, - mappings); - resultMappings.addAll(Objects.requireNonNullElse(autoMappings, mappings)); + final ResultMappingConstructorResolver resolver = new ResultMappingConstructorResolver(configuration, mappings, + resultType, id); + resultMappings.addAll(resolver.resolveWithConstructor()); } private Discriminator processDiscriminatorElement(XNode context, Class resultType, diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMap.java b/src/main/java/org/apache/ibatis/mapping/ResultMap.java index ed16f34b8c9..a74d30c28a0 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMap.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMap.java @@ -15,8 +15,6 @@ */ 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; @@ -24,11 +22,8 @@ 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; /** @@ -85,16 +80,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 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)); @@ -106,10 +103,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); @@ -118,99 +117,27 @@ public ResultMap build() { resultMap.hasResultMapsUsingConstructorCollection = resultMap.hasResultMapsUsingConstructorCollection || (resultMapping.getNestedQueryId() == 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 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 ambiguous constructors or 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 argNamesOfMatchingConstructor(List constructorArgNames) { - Constructor[] constructors = resultMap.type.getDeclaredConstructors(); - for (Constructor constructor : constructors) { - Class[] paramTypes = constructor.getParameterTypes(); - if (constructorArgNames.size() == paramTypes.length) { - List paramNames = getArgNames(constructor); - if (constructorArgNames.containsAll(paramNames) - && argTypesMatch(constructorArgNames, paramTypes, paramNames)) { - return paramNames; - } - } - } - return null; - } - - private boolean argTypesMatch(final List constructorArgNames, Class[] paramTypes, - List 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 getArgNames(Constructor constructor) { - List paramNames = new ArrayList<>(); - List 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; } } diff --git a/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java b/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java new file mode 100644 index 00000000000..9880e635abe --- /dev/null +++ b/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java @@ -0,0 +1,345 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.builder; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.AssertionsForClassTypes.tuple; +import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat; + +import java.sql.CallableStatement; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.time.LocalDate; +import java.util.Arrays; +import java.util.List; +import java.util.Map; + +import org.apache.ibatis.annotations.Param; +import org.apache.ibatis.mapping.ResultMapping; +import org.apache.ibatis.session.Configuration; +import org.apache.ibatis.type.BaseTypeHandler; +import org.apache.ibatis.type.JdbcType; +import org.assertj.core.api.Assertions; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +class ResultMappingConstructorResolverTest { + + static String TEST_ID = "testResultMapId"; + + Configuration configuration = new Configuration(); + + @Test + void testResolvesSingleArg() { + ResultMapping mapping = createConstructorMappingFor(Object.class, "type", "type"); + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType.class, TEST_ID, mapping); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, m -> m.getJavaType().getSimpleName()) + .containsExactly(tuple("type", "String")); + } + + @Test + void testResolvesTypeAndOrderWithSingleConstructor() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, "b1", "b1"); + ResultMapping mappingC = createConstructorMappingFor(Object.class, "c", "c"); + + // note the incorrect order provided here + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType2.class, TEST_ID, mappingC, mappingA, + mappingB); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple("a", "long"), tuple("b1", "long"), tuple("c", "String")); + } + + @Test + void testCannotResolveAmbiguous() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, "b", "b"); + ResultMapping mappingC = createConstructorMappingFor(Object.class, "c", "c"); + + // there are two matching constructors here, we need to clarify with type info + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingA, mappingB, + mappingC); + + assertThatThrownBy(resolver::resolveWithConstructor).isNotNull().isInstanceOf(BuilderException.class) + .hasMessageContaining( + "Failed to find a constructor in 'org.apache.ibatis.builder.ResultType1' with arg names [a, b, c]"); + } + + @Test + void testCanResolveAmbiguousWithMinimalTypeInfo() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, "b", "b"); + ResultMapping mappingC = createConstructorMappingFor(LocalDate.class, "c", "c"); + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingA, mappingB, + mappingC); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple("a", "long"), tuple("b", "String"), tuple("c", "LocalDate")); + } + + @Test + void testCanResolveAmbiguousWithAllTypeInfo() { + ResultMapping mappingA = createConstructorMappingFor(long.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(String.class, "b", "b"); + ResultMapping mappingC = createConstructorMappingFor(LocalDate.class, "c", "c"); + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingA, mappingB, + mappingC); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple("a", "long"), tuple("b", "String"), tuple("c", "LocalDate")); + } + + @Test + void testCanResolveAmbiguousRandomOrderWithMinimalTypeInfo() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, "b", "b"); + ResultMapping mappingC = createConstructorMappingFor(LocalDate.class, "c", "c"); + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingC, mappingA, + mappingB); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple("a", "long"), tuple("b", "String"), tuple("c", "LocalDate")); + } + + @Test + void testCanResolveAmbiguousRandomOrderWithAllTypeInfo() { + ResultMapping mappingA = createConstructorMappingFor(long.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(String.class, "b", "b"); + ResultMapping mappingC = createConstructorMappingFor(LocalDate.class, "c", "c"); + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingC, mappingA, + mappingB); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple("a", "long"), tuple("b", "String"), tuple("c", "LocalDate")); + } + + @Test + void testCanResolveOutOfOrderWhenParamIsUsed() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, "a1", "a1"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, "b1", "b1"); + ResultMapping mappingC = createConstructorMappingFor(Object.class, "c1", "c1"); + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingC, mappingA, + mappingB); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple("a1", "long"), tuple("b1", "long"), tuple("c1", "String")); + } + + @Test + void doesNotResolveWithNoMappingsAsInput() { + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID); + Assertions.assertThat(resolver.resolveWithConstructor()).isNotNull().isEmpty(); + } + + @Test + void testReturnOriginalMappingsWhenNoPropertyNamesDefinedAndCannotResolveConstructor() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, null, "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, null, "b"); + ResultMapping mappingC = createConstructorMappingFor(Object.class, null, "c"); + ResultMapping[] constructorMappings = new ResultMapping[] { mappingA, mappingB, mappingC }; + + // [backwards-compatibility] the mappings do not have type info, or name defined, the original mappings should be + // returned + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, + constructorMappings); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).containsExactly(constructorMappings); + } + + @Test + void testCanResolveWithMissingPropertyNameAndPartialTypeInfo() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, null, "b"); + ResultMapping mappingC = createConstructorMappingFor(LocalDate.class, null, "c"); + ResultMapping[] constructorMappings = new ResultMapping[] { mappingA, mappingB, mappingC }; + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, + constructorMappings); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple("a", "long"), tuple(null, "String"), tuple(null, "LocalDate")); + } + + @Test + void testCanResolveWithMissingPropertyNameAndAllTypeInfo() { + ResultMapping mappingA = createConstructorMappingFor(long.class, null, "a"); + ResultMapping mappingB = createConstructorMappingFor(String.class, null, "b"); + ResultMapping mappingC = createConstructorMappingFor(String.class, null, "c"); + ResultMapping[] constructorMappings = new ResultMapping[] { mappingA, mappingB, mappingC }; + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, + constructorMappings); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) + .containsExactly(tuple(null, "long"), tuple(null, "String"), tuple(null, "String")); + } + + @Test + void doesNotChangeCustomTypeHandlerAfterAutoTypeAndOrdering() { + ResultMapping mappingA = createConstructorMappingFor(String.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, "b", "b"); + ResultMapping mappingC = new ResultMapping.Builder(configuration, "c", "c", Object.class) + .typeHandler(new MyTypeHandler()).build(); + + final ResultMappingConstructorResolver resolver = createResolverFor(CustomObj.class, TEST_ID, mappingB, mappingA, + mappingC); + final List mappingList = resolver.resolveWithConstructor(); + + assertThat(mappingList) + .extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName(), + mapping -> mapping.getTypeHandler().getClass().getSimpleName()) + .containsExactly(tuple("a", "String", "StringTypeHandler"), tuple("b", "int", "IntegerTypeHandler"), + tuple("c", "List", "MyTypeHandler")); + } + + @Nested + class MetaInfoTests { + + @Test + void resolvesEmptyConstructor() { + List constructorMetaInfos = new ResultMappingConstructorResolver( + configuration, List.of(), Result.class, TEST_ID).retrieveConstructorCandidates(0); + + Assertions.assertThat(constructorMetaInfos).isNotNull().hasSize(1); + + ResultMappingConstructorResolver.ConstructorMetaInfo constructorMetaInfo = constructorMetaInfos.get(0); + Assertions.assertThat(constructorMetaInfo.getArgByOriginalIndex(0)).isNull(); + Assertions.assertThat(constructorMetaInfo.constructorArgs).isEmpty(); + } + + @Test + void resolvesNormalConstructor() { + List constructorMetaInfos = new ResultMappingConstructorResolver( + configuration, List.of(), ResultType.class, TEST_ID).retrieveConstructorCandidates(1); + + assertThat(constructorMetaInfos).isNotNull().hasSize(1).satisfiesExactlyInAnyOrder( + metaInfo0 -> assertThat(metaInfo0.constructorArgs).extractingFromEntries(Map.Entry::getKey, + entry -> entry.getValue().getType(), entry -> entry.getValue().getName()) + .containsExactly(tuple("type", String.class, "type"))); + } + + @Test + void resolvesConstructorsWithParams() { + List constructorMetaInfos = new ResultMappingConstructorResolver( + configuration, List.of(), ResultType1.class, TEST_ID).retrieveConstructorCandidates(3); + + assertThat(constructorMetaInfos).isNotNull().hasSize(3).satisfiesExactlyInAnyOrder( + metaInfo0 -> assertThat(metaInfo0.constructorArgs).extractingFromEntries(Map.Entry::getKey, + entry -> entry.getValue().getType(), entry -> entry.getValue().getName()).containsExactly( + tuple("a1", long.class, "a1"), tuple("b1", long.class, "b1"), tuple("c1", String.class, "c1")), + metaInfo1 -> assertThat(metaInfo1.constructorArgs).extractingFromEntries(Map.Entry::getKey, + entry -> entry.getValue().getType(), entry -> entry.getValue().getName()).containsExactly( + tuple("a", long.class, "a"), tuple("b", String.class, "b"), tuple("c", String.class, "c")), + metaInfo1 -> assertThat(metaInfo1.constructorArgs).extractingFromEntries(Map.Entry::getKey, + entry -> entry.getValue().getType(), entry -> entry.getValue().getName()).containsExactly( + tuple("a", long.class, "a"), tuple("b", String.class, "b"), tuple("c", LocalDate.class, "c"))); + } + + @Test + void resolvesConstructorsWithMixedParams() { + List constructorMetaInfos = new ResultMappingConstructorResolver( + configuration, List.of(), ResultType2.class, TEST_ID).retrieveConstructorCandidates(3); + + assertThat(constructorMetaInfos).isNotNull().hasSize(1).satisfiesExactlyInAnyOrder( + metaInfo0 -> assertThat(metaInfo0.constructorArgs).extractingFromEntries(Map.Entry::getKey, + entry -> entry.getValue().getType(), entry -> entry.getValue().getName()).containsExactly( + tuple("a", long.class, "a"), tuple("b1", long.class, "b1"), tuple("c", String.class, "c"))); + } + } + + private ResultMappingConstructorResolver createResolverFor(Class resultType, String identifier, + ResultMapping... mappings) { + return new ResultMappingConstructorResolver(configuration, mappings == null ? List.of() : Arrays.asList(mappings), + resultType, identifier); + } + + private ResultMapping createConstructorMappingFor(Class javaType, String property, String column) { + return new ResultMapping.Builder(configuration, property, column, javaType).build(); + } +} + +record Result() { +} + +record ResultType(String type) { +} + +record ResultType1(long a, String b, String c) { + + ResultType1(@Param("a1") long a, @Param("b1") long b, @Param("c1") String c) { + this(a, c, c); + } + + ResultType1(long a, String b, LocalDate c) { + this(a, b, c.toString()); + } + + ResultType1(long a, String b, LocalDate c, String d) { + this(a, b, c.toString()); + } +} + +record ResultType2(long a, @Param("b1") long b, String c) { +} + +class CustomObj { + CustomObj(String a, int b, List c) { + + } +} + +class MyTypeHandler extends BaseTypeHandler> { + + @Override + public void setNonNullParameter(PreparedStatement ps, int i, List parameter, JdbcType jdbcType) + throws SQLException { + + } + + @Override + public List getNullableResult(ResultSet rs, String columnName) throws SQLException { + return List.of(); + } + + @Override + public List getNullableResult(ResultSet rs, int columnIndex) throws SQLException { + return List.of(); + } + + @Override + public List getNullableResult(CallableStatement cs, int columnIndex) throws SQLException { + return List.of(); + } +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java index 1480b2beeb2..7323d49574f 100644 --- a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account1.java @@ -16,7 +16,7 @@ package org.apache.ibatis.submitted.auto_type_from_non_ambiguous_constructor; public record Account1(long accountId, String accountName, String accountType) { - public Account1(long accountId, String accountName, int accountTypeId) { - this(accountId, accountName, String.valueOf(accountTypeId)); + public Account1(long accountId, String accountName, int accountType) { + this(accountId, accountName, String.valueOf(accountType)); } } diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account4.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account4.java new file mode 100644 index 00000000000..8d27da5fbf1 --- /dev/null +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Account4.java @@ -0,0 +1,21 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://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.auto_type_from_non_ambiguous_constructor; + +import java.time.LocalDate; + +public record Account4(long accountId, String accountName, LocalDate accountDob) { +} diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java index f7476b0e498..1c5407458f8 100644 --- a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/AutoTypeFromNonAmbiguousConstructorTest.java @@ -103,4 +103,22 @@ void testChoosesCorrectConstructorWhenPartialTypesAreProvided() { } } + @Test + void testSucceedsWhenConstructorArgsAreInWrongOrderAndTypesAreNotProvided() { + try (SqlSession sqlSession = sqlSessionFactory.openSession()) { + Mapper mapper = sqlSession.getMapper(Mapper.class); + Account4 account = mapper.getAccountWrongOrder(1); + Assertions.assertThat(account).isNotNull(); + Assertions.assertThat(account.accountId()).isEqualTo(1); + Assertions.assertThat(account.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account.accountDob()).isEqualTo("2025-01-05"); + + Mapper1 mapper1 = sqlSession.getMapper(Mapper1.class); + Account4 account4 = mapper1.getAccountWrongOrder(1); + Assertions.assertThat(account4).isNotNull(); + Assertions.assertThat(account4.accountId()).isEqualTo(1); + Assertions.assertThat(account4.accountName()).isEqualTo("Account 1"); + Assertions.assertThat(account4.accountDob()).isEqualTo("2025-01-05"); + } + } } diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java index 36d33ee3a8c..349aed08c95 100644 --- a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.java @@ -25,4 +25,5 @@ public interface Mapper { Account3 getAccountPartialTypesProvided(long id); + Account4 getAccountWrongOrder(int i); } diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java index b0575544754..d7439bd58af 100644 --- a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java @@ -24,12 +24,21 @@ public interface Mapper1 { String SELECT_SQL = "select a.id, a.name, a.type from account a where a.id = #{id}"; + String SELECT_WITH_DOB_SQL = "select id, name, type, date '2025-01-05' dob from account where id = #{id}"; - @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name"), @Arg(column = "type"), }) + @ConstructorArgs({ @Arg(column = "id", name = "accountId"), @Arg(column = "name", name = "accountName"), + @Arg(column = "type", name = "accountType"), }) @Select(SELECT_SQL) Account getAccountJavaTypesMissing(long id); - @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name", javaType = String.class), @Arg(column = "type"), }) + @ConstructorArgs({ @Arg(column = "id", name = "accountId"), + @Arg(column = "name", name = "accountName", javaType = String.class), + @Arg(column = "type", name = "accountType"), }) @Select(SELECT_SQL) Account3 getAccountPartialTypesProvided(long id); + + @ConstructorArgs({ @Arg(column = "name", name = "accountName"), @Arg(column = "dob", name = "accountDob"), + @Arg(column = "id", name = "accountId") }) + @Select(SELECT_WITH_DOB_SQL) + Account4 getAccountWrongOrder(long id); } diff --git a/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml index d4c890a4246..d07844b9d6a 100644 --- a/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml +++ b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml @@ -74,4 +74,17 @@ + + + + + + + + + + + \ No newline at end of file From 7b3e6d26c46f273d74b57a7bbf83d8f983f99369 Mon Sep 17 00:00:00 2001 From: Willie Scholtz Date: Mon, 6 Jan 2025 16:04:08 +0100 Subject: [PATCH 05/12] fix: isEmpty instead of length() == 0 in MapperAnnotationBuilder --- .../annotation/MapperAnnotationBuilder.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java b/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java index d9b349ab7c1..a0d802f3d19 100644 --- a/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java +++ b/src/main/java/org/apache/ibatis/builder/annotation/MapperAnnotationBuilder.java @@ -462,7 +462,7 @@ private void applyResults(Result[] results, Class resultType, List 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(".")) { @@ -499,19 +499,19 @@ 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 resultMappings, @@ -539,7 +539,7 @@ private void applyConstructorArgs(Arg[] args, Class resultType, List Date: Sun, 12 Jan 2025 10:19:23 +0100 Subject: [PATCH 06/12] Add missing field in copy constructor --- src/main/java/org/apache/ibatis/mapping/ResultMapping.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java index 47c09f90400..bea2a3d9ad5 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java @@ -88,6 +88,7 @@ public Builder(ResultMapping otherMapping) { resultMapping.columnPrefix = otherMapping.columnPrefix; resultMapping.resultSet = otherMapping.resultSet; resultMapping.foreignColumn = otherMapping.foreignColumn; + resultMapping.lazy = otherMapping.lazy; } public Builder javaType(Class javaType) { From 90cf12455cdc4bad0753a41ccf92408ad3a38826 Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Thu, 9 Jan 2025 07:02:57 +0900 Subject: [PATCH 07/12] Remove unused Log reference --- src/main/java/org/apache/ibatis/mapping/ResultMap.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMap.java b/src/main/java/org/apache/ibatis/mapping/ResultMap.java index dfb335c3e0f..145d053fcc4 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMap.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMap.java @@ -22,8 +22,6 @@ import java.util.Locale; import java.util.Set; -import org.apache.ibatis.logging.Log; -import org.apache.ibatis.logging.LogFactory; import org.apache.ibatis.session.Configuration; /** @@ -50,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 resultMappings) { From d17b3fb4272d74f39b3f8f1b5577ebddd8ab99b5 Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Sun, 12 Jan 2025 06:41:48 +0900 Subject: [PATCH 08/12] Deprecate public methods (just for formality) --- .../java/org/apache/ibatis/mapping/ResultMapping.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java index bea2a3d9ad5..a0968a8cf64 100644 --- a/src/main/java/org/apache/ibatis/mapping/ResultMapping.java +++ b/src/main/java/org/apache/ibatis/mapping/ResultMapping.java @@ -262,10 +262,20 @@ public String getForeignColumn() { return foreignColumn; } + @Deprecated + public void setForeignColumn(String foreignColumn) { + this.foreignColumn = foreignColumn; + } + public boolean isLazy() { return lazy; } + @Deprecated + public void setLazy(boolean lazy) { + this.lazy = lazy; + } + public boolean isSimple() { return this.nestedResultMapId == null && this.nestedQueryId == null && this.resultSet == null; } From 2c2033785bcc253ad27d474a2e25016149708c17 Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Mon, 13 Jan 2025 04:11:33 +0900 Subject: [PATCH 09/12] Remove `name` attribute from where it is irrelevant --- .../Mapper1.java | 7 ++---- .../Mapper.xml | 24 +++++++++---------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java index d7439bd58af..053bdbe0aad 100644 --- a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java @@ -26,14 +26,11 @@ public interface Mapper1 { String SELECT_SQL = "select a.id, a.name, a.type from account a where a.id = #{id}"; String SELECT_WITH_DOB_SQL = "select id, name, type, date '2025-01-05' dob from account where id = #{id}"; - @ConstructorArgs({ @Arg(column = "id", name = "accountId"), @Arg(column = "name", name = "accountName"), - @Arg(column = "type", name = "accountType"), }) + @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name"), @Arg(column = "type")}) @Select(SELECT_SQL) Account getAccountJavaTypesMissing(long id); - @ConstructorArgs({ @Arg(column = "id", name = "accountId"), - @Arg(column = "name", name = "accountName", javaType = String.class), - @Arg(column = "type", name = "accountType"), }) + @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name", javaType = String.class), @Arg(column = "type")}) @Select(SELECT_SQL) Account3 getAccountPartialTypesProvided(long id); diff --git a/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml index d07844b9d6a..584eacaaa6d 100644 --- a/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml +++ b/src/test/resources/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper.xml @@ -21,33 +21,33 @@ - - - + + + - - - + + + - - - + + + - - - + + + From be848e4d7f83b7c440ae344d1872420f0ace712b Mon Sep 17 00:00:00 2001 From: Willie Scholtz Date: Wed, 15 Jan 2025 13:05:21 +0100 Subject: [PATCH 10/12] fix(#2932): remove ability to partially specify property name for constructor args - see https://github.com/mybatis/mybatis-3/pull/3378#discussion_r1913948789 --- .../builder/ResultMappingConstructorResolver.java | 15 ++++++++++++++- .../ResultMappingConstructorResolverTest.java | 12 +++++------- .../Mapper1.java | 4 ++-- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java index 4fe821fc170..95347a2f478 100644 --- a/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java +++ b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java @@ -103,7 +103,7 @@ public List resolveWithConstructor() { .filter(Objects::nonNull).collect(Collectors.toCollection(LinkedHashSet::new)); // arg order can only be 'fixed' if all mappings have property names - final boolean allMappingsHavePropertyNames = constructorResultMappings.size() == constructorArgsByName.size(); + final boolean allMappingsHavePropertyNames = verifyPropertyNaming(constructorArgsByName); // only do this if all property mappings were set if (allMappingsHavePropertyNames) { @@ -147,6 +147,19 @@ public List resolveWithConstructor() { return resultMappings; } + private boolean verifyPropertyNaming(Set constructorArgsByName) { + final boolean allMappingsHavePropertyNames = constructorResultMappings.size() == constructorArgsByName.size(); + + // If property names have been partially specified, throw an exception, as this case does not make sense + // either specify all names and (optional random order), or type info. + if (!allMappingsHavePropertyNames && !constructorArgsByName.isEmpty()) { + throw new BuilderException("Error in result map '" + resultMapId + + "'. We do not support partially specifying a property name. Either specify all property names, or none."); + } + + return allMappingsHavePropertyNames; + } + List retrieveConstructorCandidates(int withLength) { return Arrays.stream(resultType.getDeclaredConstructors()) .filter(constructor -> constructor.getParameterTypes().length == withLength).map(ConstructorMetaInfo::new) diff --git a/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java b/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java index 9880e635abe..dce9990aa63 100644 --- a/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java +++ b/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java @@ -177,18 +177,16 @@ void testReturnOriginalMappingsWhenNoPropertyNamesDefinedAndCannotResolveConstru } @Test - void testCanResolveWithMissingPropertyNameAndPartialTypeInfo() { + void testThrowExceptionWithPartialPropertyNameSpecified() { ResultMapping mappingA = createConstructorMappingFor(Object.class, "a", "a"); ResultMapping mappingB = createConstructorMappingFor(Object.class, null, "b"); ResultMapping mappingC = createConstructorMappingFor(LocalDate.class, null, "c"); - ResultMapping[] constructorMappings = new ResultMapping[] { mappingA, mappingB, mappingC }; - final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, - constructorMappings); - final List mappingList = resolver.resolveWithConstructor(); + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingA, mappingB, + mappingC); - assertThat(mappingList).extracting(ResultMapping::getProperty, mapping -> mapping.getJavaType().getSimpleName()) - .containsExactly(tuple("a", "long"), tuple(null, "String"), tuple(null, "LocalDate")); + assertThatThrownBy(resolver::resolveWithConstructor).isInstanceOf(BuilderException.class) + .hasMessageContaining("Either specify all property names, or none."); } @Test diff --git a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java index 053bdbe0aad..ed595439cb2 100644 --- a/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java +++ b/src/test/java/org/apache/ibatis/submitted/auto_type_from_non_ambiguous_constructor/Mapper1.java @@ -26,11 +26,11 @@ public interface Mapper1 { String SELECT_SQL = "select a.id, a.name, a.type from account a where a.id = #{id}"; String SELECT_WITH_DOB_SQL = "select id, name, type, date '2025-01-05' dob from account where id = #{id}"; - @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name"), @Arg(column = "type")}) + @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name"), @Arg(column = "type") }) @Select(SELECT_SQL) Account getAccountJavaTypesMissing(long id); - @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name", javaType = String.class), @Arg(column = "type")}) + @ConstructorArgs({ @Arg(column = "id"), @Arg(column = "name", javaType = String.class), @Arg(column = "type") }) @Select(SELECT_SQL) Account3 getAccountPartialTypesProvided(long id); From f801726138da7e3af5f11ec157a0fca2cc22192a Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Sat, 25 Jan 2025 08:47:03 +0900 Subject: [PATCH 11/12] Minor adjustments for cases where constructor mapping does not use actual constructor --- .../builder/ResultMappingConstructorResolver.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java index 95347a2f478..7268471e79c 100644 --- a/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java +++ b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java @@ -98,6 +98,10 @@ public List resolveWithConstructor() { final List matchingConstructorCandidates = retrieveConstructorCandidates( constructorResultMappings.size()); + if (matchingConstructorCandidates.isEmpty()) { + return constructorResultMappings; + } + // extract the property names we have final Set constructorArgsByName = constructorResultMappings.stream().map(ResultMapping::getProperty) .filter(Objects::nonNull).collect(Collectors.toCollection(LinkedHashSet::new)); @@ -123,11 +127,9 @@ public List resolveWithConstructor() { + ". Note that 'javaType' is required when there is ambiguous constructors or there is no writable property with the same name ('name' is optional, BTW). There is more info in the debug log."); } else { if (log.isDebugEnabled()) { - log.debug("Constructor for '" + resultMapId - + "' could not be resolved, continuing, but this may result in a mapping exception later"); + log.debug("Constructor for '" + resultMapId + "' could not be resolved."); } - // return un-modified original mappings (maybe have this as a config flag, as this will result in a runtime - // exception eventually + // return un-modified original mappings return constructorResultMappings; } } From add7b2647a556013abb1231679ca72a20d17ba91 Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Sat, 25 Jan 2025 09:30:08 +0900 Subject: [PATCH 12/12] Improved error message that might save an exhausted developer dealing with a monstrous constructor --- .../builder/ResultMappingConstructorResolver.java | 2 +- .../ResultMappingConstructorResolverTest.java | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java index 7268471e79c..223261c3fd5 100644 --- a/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java +++ b/src/main/java/org/apache/ibatis/builder/ResultMappingConstructorResolver.java @@ -156,7 +156,7 @@ private boolean verifyPropertyNaming(Set constructorArgsByName) { // either specify all names and (optional random order), or type info. if (!allMappingsHavePropertyNames && !constructorArgsByName.isEmpty()) { throw new BuilderException("Error in result map '" + resultMapId - + "'. We do not support partially specifying a property name. Either specify all property names, or none."); + + "'. We do not support partially specifying a property name nor duplicates. Either specify all property names, or none."); } return allMappingsHavePropertyNames; diff --git a/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java b/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java index dce9990aa63..ac1912c09d9 100644 --- a/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java +++ b/src/test/java/org/apache/ibatis/builder/ResultMappingConstructorResolverTest.java @@ -189,6 +189,19 @@ void testThrowExceptionWithPartialPropertyNameSpecified() { .hasMessageContaining("Either specify all property names, or none."); } + @Test + void testThrowExceptionWithDuplicatedPropertyNames() { + ResultMapping mappingA = createConstructorMappingFor(Object.class, "a", "a"); + ResultMapping mappingB = createConstructorMappingFor(Object.class, "a", "b"); + ResultMapping mappingC = createConstructorMappingFor(LocalDate.class, "c", "c"); + + final ResultMappingConstructorResolver resolver = createResolverFor(ResultType1.class, TEST_ID, mappingA, mappingB, + mappingC); + + assertThatThrownBy(resolver::resolveWithConstructor).isInstanceOf(BuilderException.class) + .hasMessageContaining("Either specify all property names, or none."); + } + @Test void testCanResolveWithMissingPropertyNameAndAllTypeInfo() { ResultMapping mappingA = createConstructorMappingFor(long.class, null, "a");