TypeHandler incorrectly selects super classes/interfaces

[matthewmillett-instaclustr]

MyBatis version

3.5.8 (any version above 3.4.2)

Database vendor and version

Any (tested on Postgres 12.7 and latest JDBC, but also fails in testing framework)

Test case or example project

https://github.com/matthewmillett-instaclustr/mybatis-issues/tree/master/gh-2427

Steps to reproduce

Run tests

Expected result

shouldGetAUser() passes, by using the constructor within the User class to construct the User object

shouldGetAnAccount() passes (regression check)

Actual result

shouldGetAUser() fails with the exception java.lang.ClassCastException: class test.UserPrimaryKey cannot be cast to class test.User (test.UserPrimaryKey and test.User are in unnamed module of loader 'app')

shouldGetAnAccount() passes (regression check)

Explanation

I believe that this issue was introduced with https://github.com/mybatis/mybatis-3/issues/604 (PR https://github.com/mybatis/mybatis-3/pull/859). Before 3.4.2, Mybatis would use the type User, and find the constructor within the User class to build the result object. However, in 3.4.2 and above, Mybatis attempts to search the super class UserPrimaryKey, and uses the constructor within the UserPrimaryKey class to build a User object. It builds the UserPrimaryKey object fine, but then hits an error when attempting to cast it to the expected result object of User.

Sample fix

This is far from an ideal solution, but proves to show that the above explanation holds true.

Within org.apache.ibatis.type.TypeHandlerRegistry#getJdbcHandlerMapForSuperclass, if we add the following check after the call to clazz.getSuperclass(), the code works as intended:

if (clazz.getSuperclass() != null && clazz.getSuperclass().getName().contains("PrimaryKey")) {
        return typeHandlerMap.get(clazz);
}

An alternative way of resolving this may be to check for assignability within the above function, and only if the super class is assignable to the subclass then return the typeHandlerMap of the super class. I'm not sure how backwards compatible this would be, and/or if it would work.

Other

I'm pretty sure this is not a configuration problem, but if it is, then please let me know. If it can be resolved with the correct annotations within the mapper, I will be willing to give it a go. I couldn't see any other related tickets with a similar issue. I know the hierarchy of User extending UserPrimaryKey implementing PrimaryKey is a bit odd, but modifying our inheritance/implementation structure is not an option.

[harawata]

Hello @matthewmillett-instaclustr ,

Thank you for providing the demo!

Type handler is unnecessary for that mapping and it actually causes the error in your case, so just remove the type handler and its config.

I also noticed a few minor mistakes.

• For the Mapper#getUserByName, parameter reference #{user.name} does not match the parameter type i.e. String.
• In the SimpleTest, secondUser is used in the assertions for thirdUser and fourthUser.

The tests passed after fixing these issues. Please let us know if you need any further clarification.

[matthewmillett-instaclustr]

Hi @harawata,

Thanks for the quick reply! Apologies for the minor mistakes in my demo code.

I've had a bit of a look into your suggestions. I removed the type handler config from mybatis-config.xml, and the tests ran fine. However; I realised that there is another related problem that wasn't captured earlier.

The other problem is when we need the type handler for handling an object from the DB, for example expecting a UserPrimaryKey being returned from a SELECT statement. I've added a new test (shouldGetACompany) in my demo code to demonstrate this.

When the UserPrimaryKeyHandler is enabled/included within the config, shouldGetAUser fails, and other tests pass.

When the UserPrimaryKeyHandler is disabled/removed from the config, the Mybatis configuration fails to build due to not finding a typehandler for property null (it shows in the logs as null, but after debugging it's looking for a handler for the java type UserPrimaryKey).

A solution to this I've investigated would be to create a type handler for User, and keep the type handler for UserPrimaryKey. Some disadvantages to this include:

• Details on constructing a user would need to be redefined in UserTypeHandler, and effort will need to be made to ensure that annotations naming remains constant (this isn't a big problem for our use case, but it is a bit messy)
• Type handlers would need to be created for all classes implementing/extending implementations of PrimaryKey (this is a bit more of a problem for us)

There is also a chance I'm writing the UserTypeHandler incorrectly/not in alignment with best practices, and if that is the case I'd appreciate some feedback.

Thanks!

[matthewmillett-instaclustr]

Hi @harawata,

We've had a bit more of a look at the above potential solutions. We've found that if we change the logic within DefaultResultSetHandler to check for constructor mappings before checking for type handlers, we can include annotations within the mapper class itself, next to the SQL we are running, instead of creating new type handlers for all objects that extend PrimaryKey.

This change can be seen in https://github.com/mybatis/mybatis-3/pull/2430.

The only regression we can potentially see is if someone has redundantly (and incorrectly) specified constructor args for a type which already has a type handler. In this case, the incorrect constructor args would be used as opposed to the type handler.

An example of this error can be seen within the test repo, under shouldGetUserAdmin - it produces an error Error querying database. Cause: org.apache.ibatis.reflection.ReflectionException: Error instantiating boolean with invalid types (boolean) or values (false). Cause: java.lang.NoSuchMethodException: boolean.<init>(boolean). This error goes away if the erroneous constructor mappings are removed for that function.

I've also added in some small opportunistic changes within the PR to improve exceptions within validation of ResultMap - I found that the current logging of only the property attribute wasn't too helpful, but including the javaType improved context.

Let me know what you think about the proposed changes.

Thanks

[harawata]

Hi @matthewmillett-instaclustr ,

I'm sorry about the slow response and thank you for the PR!

When your type handler is not applicable to the subclasses of the explicitly-mapped type, the type handler should not be registered globally. Instead, you need to specify typeHandler attribute explicitly.

The test passes if you change the mapping of Mapper#getCompanyByName() as follows.

@Select("...")
@ConstructorArgs({
  // ...
  @Arg(
    column = "main_user", 
    javaType = UserPrimaryKey.class, 
    typeHandler = UserPrimaryKeyHandler.class)
})
Company getCompanyByName(@Param("companyName") final String companyName);

I took a look at #2430 , but the scope of the underlying issue is wider and the proposed fix addresses only a part of it.

[matthewmillett-instaclustr]

Hi @harawata,

Thanks for the suggestion to use typeHandler = ... within ConstructorArgs. After modifying our codebase quite a bit we've managed to get things mostly working, although it's not as clean as we'd hoped.

I've spoken with my team, and our reasoning behind the change proposed in #2430 was that when the constructor args are specified explicitly there should be no need to be searching for a typehandler. Additionally, overriding the explicitly specified constructor args doesn’t seems like desirable behaviour.

If that reasoning is incorrect or there are deeper implications of the proposed change we are happy to investigate further and work with the team to try to come up with a sound solution. We understand that changes such as these can lead to backwards compatibility issues for people who use the library; however, at the same time, resolving these issues assists with reducing technical debt, and improves the developer experience.

Let us know how the above sounds.

Thanks