Fix IndexOutOfBoundsException

[luozongle01]

Hi, I have noticed that some people, when using Lombok, only have one constructor with all parameters due to the use of @Data and @Builder annotations. If the class property of the resultType does not match the query result, an IndexOutOfBoundsException exception will be thrown.
I have had several colleagues who have encountered this issue and spent some time troubleshooting it. Although it is indeed the user's problem, I think perhaps we should optimize the prompt to clearly indicate that this is the cause of the constructor, so that users can quickly find the problem when they encounter it.

Abnormal example: https://github.com/luozongle01/mybatis_index_out_of_bounds

The following is the abnormal prompt after the problem occurs

The following is the modified exception prompt

[harawata]

Thank you for the PR, @luozongle01 ,

I agree that MyBatis should avoid throwing IndexOutOfBoundsException, however, calling findUsableConstructorByArgTypes() could break backward compatibility (type handler is not always registered globally).
Just checking the number of constructor arguments should be enough, am I right?

Also, for the tests, please create a new independent package under the org.apache.ibatis.submitted and use HSQLDB like the other tests instead of mocking.

p.s.
Personally, I would recommend using the argNameBasedConstructorAutoMapping if you rely on constructor auto-mapping.
Please see #2196 for the details.

[harawata]

Calling findUsableConstructorByArgTypes() might not break backward compatibility.
You can keep it as-is for now. I'll look into it later.

Please make the requested change for the tests.

[luozongle01]

Calling findUsableConstructorByArgTypes() might not break backward compatibility. You can keep it as-is for now. I'll look into it later.

Please make the requested change for the tests.

Hi @harawata , thank you very much for your review. I will modify the test.

However, I found that my changes caused the build test to fail, and then I found some problems that my changes may cause

These only appear when the resultType class has only one constructor with parameters.

1. When argNameBasedConstructorAutoMapping is false, when the number of sql query result columns is greater than the number of constructor parameters, it was allowed before, but now it will fail.

2. When argNameBasedConstructorAutoMapping is true, when the number of sql query result columns is greater than the number of constructor parameters, it was allowed before, but now it will fail.

3. When argNameBasedConstructorAutoMapping is true, when the sql query result column type and the constructor order are inconsistent, it was allowed before, but now it will fail.

So I'm wondering if I should make changes similar to the following to keep the previous behavior?

  private Optional<? extends Constructor<?>> findConstructorForAutomapping(final Class<?> resultType,
      ResultSetWrapper rsw) {
    Constructor<?>[] constructors = resultType.getDeclaredConstructors();
    if (constructors.length == 1) {

      int constructorsParameterLength = constructors[0].getParameterTypes().length;
      if (constructorsParameterLength > rsw.getJdbcTypes().size()) {
        return Optional.empty();
      }

      if (configuration.isArgNameBasedConstructorAutoMapping() ||
        constructorsParameterLength < rsw.getJdbcTypes().size()) {
        return Optional.of(constructors[0]);
      }

      return Optional.of(constructors[0]).filter(x -> findUsableConstructorByArgTypes(x, rsw.getJdbcTypes()));
    }

    Other unchanged parts...
}

[hazendaz]

IMO using lombok with one constructor with all attributes is poorly designed usage, regardless they need the lombok noArgConstructor as using one with all attributes loses that unless directly called which new javadocs and other aspects of jvm is no requiring users be specific and not have auto generated constructors at all. Anyway, open for improvement here but think as noted users are at fault for poor design of their objects.

[luozongle01]

IMO using lombok with one constructor with all attributes is poorly designed usage, regardless they need the lombok noArgConstructor as using one with all attributes loses that unless directly called which new javadocs and other aspects of jvm is no requiring users be specific and not have auto generated constructors at all. Anyway, open for improvement here but think as noted users are at fault for poor design of their objects.

Yes, you are right, and the embarrassing thing is that some users do not notice that using the @Builder annotation will result in them not having a no-argument constructor. 😂😂

[harawata]

@luozongle01 ,

Thanks for the reply!
I will comment within a day or two.

[luozongle01]

@luozongle01 ,

Thanks for the reply! I will comment within a day or two.

@harawata
OK, thank you very much.

[harawata]

@luozongle01 ,

I think the array sizes can and should be checked in the applyColumnOrderBasedConstructorAutomapping() method, from where the IndexOutOfBoundsException is thrown.

mybatis-3/src/main/java/org/apache/ibatis/executor/resultset/DefaultResultSetHandler.java

Lines 783 to 796 in 3e54e62

	private boolean applyColumnOrderBasedConstructorAutomapping(ResultSetWrapper rsw, List<Class<?>> constructorArgTypes,
	List<Object> constructorArgs, Constructor<?> constructor, boolean foundValues) throws SQLException {
	Class<?>[] parameterTypes = constructor.getParameterTypes();
	for (int i = 0; i < parameterTypes.length; i++) {
	Class<?> parameterType = parameterTypes[i];
	String columnName = rsw.getColumnNames().get(i);
	TypeHandler<?> typeHandler = rsw.getTypeHandler(parameterType, columnName);
	Object value = typeHandler.getResult(rsw.getResultSet(), columnName);
	constructorArgTypes.add(parameterType);
	constructorArgs.add(value);
	foundValues = value != null || foundValues;
	}
	return foundValues;
	}

A few notes:

• We don't have to check the argument types. If there is a mismatch, it is the ObjectFactory's responsibility to throw appropriate exception.
• I would expect that the fix for this issue does not affect the behavior of argNameBasedConstructorAutoMapping in any way.

Please let me know if I am missing something!