Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IndexOutOfBoundsException #3327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luozongle01
Copy link
Contributor

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

image

The following is the modified exception prompt

image

Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

harawata commented Dec 25, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

luozongle01 commented Dec 26, 2024

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
Copy link
Member

@luozongle01 ,

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

@luozongle01
Copy link
Contributor Author

@luozongle01 ,

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

@harawata
OK, thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants